Repository: fmaclen/hollama (ts)

Issue:
Delete confirmation has to be clicked twice
~~It might be being fired twice.~~ Actually, pressing `ENTER` once also confirms the alert once.
[The test](https://github.com/fmaclen/hollama/commit/22adf74d5c0da17fbc94946395b5079e78013d2a#diff-b728858cc9ac6c41fb9cebc11f95efebff4b3708eefe0b569f765fee9b52998cR129) is technically also confirming the alert once so 🤷‍♂️ 

![image](https://github.com/fmaclen/hollama/assets/1434675/2c634a94-1d8a-407d-8f86-11b3335ca9f9)


Pull request:
fix: improve UI for deletion of Sessions & Knowledge
Closes #29 

Patch:
diff --git a/src/lib/components/ButtonDelete.svelte b/src/lib/components/ButtonDelete.svelte
new file mode 100644
index 0000000..d521610
--- /dev/null
+++ b/src/lib/components/ButtonDelete.svelte
@@ -0,0 +1,72 @@
+<script lang="ts">
+	import { Sitemap } from '$lib/sitemap';
+	import { Check, Trash2, X } from 'lucide-svelte';
+	import type { Writable } from 'svelte/store';
+	import { knowledgeStore, sessionsStore, deleteStoreItem } from '$lib/store';
+	import { goto } from '$app/navigation';
+
+	export let sitemap: Sitemap;
+	export let id: string;
+	export let shouldConfirmDeletion: Writable<boolean>;
+
+	function deleteRecord() {
+		$shouldConfirmDeletion = false;
+
+		switch (sitemap) {
+			case Sitemap.KNOWLEDGE:
+				if ($knowledgeStore) $knowledgeStore = deleteStoreItem($knowledgeStore, id);
+				return goto('/knowledge');
+
+			case Sitemap.SESSIONS:
+				if ($sessionsStore) $sessionsStore = deleteStoreItem($sessionsStore, id);
+				return goto('/sessions');
+
+			default:
+				break;
+		}
+	}
+
+	function updateConfirmDeletion(value: boolean) {
+		$shouldConfirmDeletion = value;
+	}
+</script>
+
+<div class="delete" class:delete--confirm-deletion={$shouldConfirmDeletion}>
+	{#if $shouldConfirmDeletion}
+		<button class="delete__confirm" on:click={deleteRecord} title="Confirm deletion">
+			<Check class="h-4 w-4" />
+		</button>
+		<button class="delete__cancel" on:click={() => updateConfirmDeletion(false)} title="Dismiss">
+			<X class="h-4 w-4" />
+		</button>
+	{:else}
+		<button
+			class="delete__trash"
+			on:click={() => updateConfirmDeletion(true)}
+			title="Delete {sitemap === Sitemap.KNOWLEDGE ? 'knowledge' : 'session'}"
+		>
+			<Trash2 class="h-4 w-4" />
+		</button>
+	{/if}
+</div>
+
+<style lang="scss">
+	.delete {
+		@apply flex h-full flex-row text-muted;
+	}
+
+	.delete__cancel,
+	.delete__confirm,
+	.delete__trash {
+		@apply px-2 py-2;
+	}
+
+	.delete__confirm {
+		@apply hover:text-negative;
+	}
+
+	.delete__trash,
+	.delete__cancel {
+		@apply hover:text-base;
+	}
+</style>
diff --git a/src/lib/components/Header.svelte b/src/lib/components/Header.svelte
index 7563896..a779a82 100644
--- a/src/lib/components/Header.svelte
+++ b/src/lib/components/Header.svelte
@@ -1,16 +1,32 @@
-<header class="header">
-	<div class="space-y-2">
+<script lang="ts">
+	export let confirmDeletion: boolean = false;
+</script>
+
+<header class="header" class:header--confirm-deletion={confirmDeletion}>
+	<div class="header__headline">
 		<slot />
 	</div>
 
-	<nav>
+	<nav class="header__nav">
 		<slot name="nav" />
 	</nav>
 </header>
 
-<style>
+<style lang="scss">
+	@import '$lib/mixins.scss';
+
 	.header {
-		@apply flex items-center justify-between border-b p-4 bg-shade-1;
+		@include delete-record-overlay;
+		@apply flex items-end justify-between border-b bg-shade-1;
+	}
+
+	.header__headline {
+		@apply flex flex-col gap-y-2 p-4;
 		@apply lg:p-6;
 	}
+
+	.header__nav {
+		@apply flex flex-row items-center gap-x-2 pr-4 pb-4;
+		@apply lg:pr-6 lg:pb-6;
+	}
 </style>
diff --git a/src/lib/components/SectionListItem.svelte b/src/lib/components/SectionListItem.svelte
index 3c45a37..fd1f31d 100644
--- a/src/lib/components/SectionListItem.svelte
+++ b/src/lib/components/SectionListItem.svelte
@@ -1,6 +1,8 @@
 <script lang="ts">
 	import { page } from '$app/stores';
 	import { Sitemap } from '$lib/sitemap';
+	import { writable } from 'svelte/store';
+	import ButtonDelete from './ButtonDelete.svelte';
 	import Metadata from './Metadata.svelte';
 
 	export let sitemap: Sitemap;
@@ -9,31 +11,68 @@
 	export let subtitle: string;
 
 	const isSession = sitemap === Sitemap.SESSIONS;
+	const shouldConfirmDeletion = writable(false);
 </script>
 
-<a
-	class="section-list-item {$page.url.pathname.includes(id) ? 'section-list-item--active' : ''}"
-	data-testid={isSession ? 'session-item' : 'knowledge-item'}
-	aria-label={isSession ? `Session: ${id}` : `Knowledge: ${id}`}
-	href={`/${sitemap}/${id}`}
->
-	<p class="section-list-item__title">
-		{title}
-	</p>
-	<Metadata>
-		{subtitle}
-	</Metadata>
-</a>
+<!-- Need to use `#key id` to re-render the delete nav after deletion -->
+{#key id}
+	<div
+		class="section-list-item"
+		class:section-list-item--active={$page.url.pathname.includes(id)}
+		class:section-list-item--confirm-deletion={$shouldConfirmDeletion}
+	>
+		<a
+			class="section-list-item__a"
+			data-testid={isSession ? 'session-item' : 'knowledge-item'}
+			aria-label={isSession ? `Session: ${id}` : `Knowledge: ${id}`}
+			href={`/${sitemap}/${id}`}
+		>
+			<p class="section-list-item__title">
+				{title}
+			</p>
+			<Metadata>
+				{subtitle}
+			</Metadata>
+		</a>
+		<nav
+			class="section-list-item__delete"
+			class:section-list-item__delete--confirm-deletion={$shouldConfirmDeletion}
+		>
+			<ButtonDelete {sitemap} {id} {shouldConfirmDeletion} />
+		</nav>
+	</div>
+{/key}
 
 <style lang="scss">
+	@import '$lib/mixins.scss';
+
 	.section-list-item {
-		@apply flex flex-col border-b px-6 py-3 last:border-b-0 hover:bg-shade-1;
+		@include delete-record-overlay;
+		@apply flex flex-row items-center justify-between border-b pr-3 ;
+		@apply last:border-b-0;
+	}
+
+	.section-list-item:hover .section-list-item__delete {
+		@apply opacity-100;
+	}
+
+	.section-list-item__delete {
+		@apply opacity-0;
+	}
+
+	.section-list-item__delete--confirm-deletion {
+		@apply opacity-100;
 	}
 
 	.section-list-item--active {
 		@apply bg-shade-1;
 	}
 
+	.section-list-item__a {
+		@apply relative z-0 w-full overflow-hidden text-ellipsis px-6 py-3;
+		@apply hover:text-active;
+	}
+
 	.section-list-item__title {
 		@apply max-w-full truncate whitespace-nowrap text-sm font-bold;
 	}
diff --git a/src/lib/mixins.scss b/src/lib/mixins.scss
index c0ab836..ecd6972 100644
--- a/src/lib/mixins.scss
+++ b/src/lib/mixins.scss
@@ -6,3 +6,9 @@
 	@apply h-full w-full border-t overflow-y-auto bg-shade-2;
 	@apply lg:rounded-xl lg:border;
 }
+
+@mixin delete-record-overlay {
+	&--confirm-deletion {
+		@apply bg-gradient-to-r from-transparent to-red-50;
+	}
+}
diff --git a/src/lib/store.ts b/src/lib/store.ts
index 5216077..5ad6aa5 100644
--- a/src/lib/store.ts
+++ b/src/lib/store.ts
@@ -34,6 +34,10 @@ export function sortStore<T extends { updatedAt?: string }>(store: T[]) {
 	});
 }
 
+export function deleteStoreItem<T extends { id: string }>(store: T[], id: string) {
+	return store.filter((s) => s.id !== id);
+}
+
 export const LOCAL_STORAGE_PREFIX = 'hollama';
 
 export interface Settings {
diff --git a/src/routes/knowledge/[id]/+page.svelte b/src/routes/knowledge/[id]/+page.svelte
index 9790e12..642720f 100644
--- a/src/routes/knowledge/[id]/+page.svelte
+++ b/src/routes/knowledge/[id]/+page.svelte
@@ -1,17 +1,18 @@
 <script lang="ts">
-	import { afterNavigate, goto } from '$app/navigation';
+	import { writable } from 'svelte/store';
+	import { afterNavigate } from '$app/navigation';
 	import type { PageData } from './$types';
-	import { Trash2 } from 'lucide-svelte';
 
-	import Button from '$lib/components/Button.svelte';
 	import { type Knowledge, loadKnowledge, saveKnowledge } from '$lib/knowledge';
 	import { formatTimestampToNow, getUpdatedAtDate } from '$lib/utils';
-	import { knowledgeStore } from '$lib/store';
+	import { Sitemap } from '$lib/sitemap';
+	import Button from '$lib/components/Button.svelte';
 	import Header from '$lib/components/Header.svelte';
 	import Fieldset from '$lib/components/Fieldset.svelte';
 	import FieldTextEditor from '$lib/components/FieldTextEditor.svelte';
 	import ButtonSubmit from '$lib/components/ButtonSubmit.svelte';
 	import FieldInput from '$lib/components/FieldInput.svelte';
+	import ButtonDelete from '$lib/components/ButtonDelete.svelte';
 	import Metadata from '$lib/components/Metadata.svelte';
 
 	export let data: PageData;
@@ -19,6 +20,7 @@
 	let knowledge: Knowledge = loadKnowledge(data.id);
 	let name: string;
 	let content: string;
+	const shouldConfirmDeletion = writable(false);
 
 	$: isNewKnowledge = !name || !content;
 
@@ -28,17 +30,6 @@
 		knowledge = loadKnowledge(data.id);
 	}
 
-	function deleteKnowledge() {
-		const confirmed = confirm('Are you sure you want to delete this knowledge?');
-		if (!confirmed) return;
-
-		if ($knowledgeStore) {
-			const updatedKnowledge = $knowledgeStore.filter((s) => s.id !== knowledge.id);
-			$knowledgeStore = updatedKnowledge;
-		}
-		goto('/knowledge');
-	}
-
 	afterNavigate(() => {
 		knowledge = loadKnowledge(data.id);
 		name = knowledge.name;
@@ -46,8 +37,8 @@
 	});
 </script>
 
-<div class="flex h-full w-full flex-col overflow-hidden">
-	<Header>
+<div class="knowledge">
+	<Header confirmDeletion={$shouldConfirmDeletion}>
 		<p data-testid="knowledge-id" class="text-sm font-bold leading-none">
 			Knowledge
 			<Button size="link" variant="link" href={`/knowledge/${knowledge.id}`}>
@@ -60,9 +51,7 @@
 
 		<svelte:fragment slot="nav">
 			{#if !isNewKnowledge}
-				<Button title="Delete knowledge" variant="outline" size="icon" on:click={deleteKnowledge}>
-					<Trash2 class="h-4 w-4" />
-				</Button>
+				<ButtonDelete sitemap={Sitemap.KNOWLEDGE} id={knowledge.id} {shouldConfirmDeletion} />
 			{/if}
 		</svelte:fragment>
 	</Header>
@@ -77,3 +66,9 @@
 		<ButtonSubmit hasMetaKey={true} {handleSubmit} disabled={!name || !content}>Save</ButtonSubmit>
 	</Fieldset>
 </div>
+
+<style lang="scss">
+	.knowledge {
+		@apply flex h-full w-full flex-col overflow-hidden;
+	}
+</style>
diff --git a/src/routes/sessions/[id]/+page.svelte b/src/routes/sessions/[id]/+page.svelte
index beb2c9a..e44b0e2 100644
--- a/src/routes/sessions/[id]/+page.svelte
+++ b/src/routes/sessions/[id]/+page.svelte
@@ -1,7 +1,7 @@
 <script lang="ts">
 	import { afterUpdate, tick } from 'svelte';
-	import { goto } from '$app/navigation';
-	import { Brain, StopCircle, Trash2, UnfoldVertical } from 'lucide-svelte';
+	import { writable } from 'svelte/store';
+	import { Brain, StopCircle, UnfoldVertical } from 'lucide-svelte';
 
 	import Button from '$lib/components/Button.svelte';
 	import Article from './Article.svelte';
@@ -11,7 +11,7 @@
 	import Header from '$lib/components/Header.svelte';
 
 	import { loadKnowledge, type Knowledge } from '$lib/knowledge';
-	import { settingsStore, sessionsStore, knowledgeStore } from '$lib/store';
+	import { settingsStore, knowledgeStore } from '$lib/store';
 	import { ollamaGenerate, type OllamaCompletionResponse } from '$lib/ollama';
 	import {
 		saveSession,
@@ -28,22 +28,24 @@
 	import ButtonSubmit from '$lib/components/ButtonSubmit.svelte';
 	import Fieldset from '$lib/components/Fieldset.svelte';
 	import Field from '$lib/components/Field.svelte';
+	import ButtonDelete from '$lib/components/ButtonDelete.svelte';
 	import Metadata from '$lib/components/Metadata.svelte';
 
 	export let data: PageData;
 
 	let messageWindow: HTMLElement;
+	let knowledgeId: string;
+	let knowledge: Knowledge | null;
 	let session: Session;
 	let completion: string;
+	let abortController: AbortController;
 	let prompt: string;
 	let promptCached: string;
-	let abortController: AbortController;
 	let promptTextarea: HTMLTextAreaElement;
 	let isPromptFullscreen = false;
 	let shouldFocusTextarea = false;
 
-	let knowledgeId: string;
-	let knowledge: Knowledge | null;
+	const shouldConfirmDeletion = writable(false);
 
 	$: session = loadSession(data.id);
 	$: isNewSession = !session?.messages.length;
@@ -80,17 +82,6 @@
 		session.messages = [...session.messages, message];
 	}
 
-	function deleteSession() {
-		const confirmed = confirm('Are you sure you want to delete this session?');
-		if (!confirmed) return;
-
-		if ($sessionsStore) {
-			const updatedSessions = $sessionsStore.filter((s) => s.id !== session.id);
-			$sessionsStore = updatedSessions;
-		}
-		goto('/sessions');
-	}
-
 	async function handleCompletionDone(completion: string, context: number[]) {
 		const message: Message = { role: 'ai', content: completion };
 		session.messages = [...session.messages, message];
@@ -183,7 +174,7 @@
 </script>
 
 <div class="session">
-	<Header>
+	<Header confirmDeletion={$shouldConfirmDeletion}>
 		<p data-testid="session-id" class="text-sm font-bold leading-none">
 			Session <Button size="link" variant="link" href={`/sessions/${session.id}`}
 				>#{session.id}</Button
@@ -195,9 +186,7 @@
 
 		<svelte:fragment slot="nav">
 			{#if !isNewSession}
-				<Button title="Delete session" variant="outline" size="icon" on:click={deleteSession}>
-					<Trash2 class="h-4 w-4" />
-				</Button>
+				<ButtonDelete sitemap={Sitemap.SESSIONS} id={session.id} {shouldConfirmDeletion} />
 			{/if}
 		</svelte:fragment>
 	</Header>


Test patch:
diff --git a/tests/knowledge.test.ts b/tests/knowledge.test.ts
index 9c84a6a..6ad1946 100644
--- a/tests/knowledge.test.ts
+++ b/tests/knowledge.test.ts
@@ -56,7 +56,30 @@ test('creates and edits knowledge', async ({ page }) => {
 	await expect(page.locator('.section-list', { hasText: "Wally's chabot" })).toBeVisible();
 });
 
-test('deletes knowledge', async ({ page }) => {
+test('can delete knowledge from the header and sidebar', async ({ page }) => {
+	await page.goto('/');
+	await page.getByText('Knowledge', { exact: true }).click();
+	expect(await page.getByTestId('knowledge-item').count()).toBe(0);
+	
+	await seedKnowledgeAndReload(page);
+	await expect(page.locator('header').getByTitle('Delete knowledge')).not.toBeVisible();
+	expect(await page.getByTestId('knowledge-item').count()).toBe(2);
+
+	// Delete the knowledge from the header
+	await page.locator('.section-list-item__a').first().click();
+	await page.locator('header').getByTitle('Delete knowledge').click();
+	await page.getByTitle('Confirm deletion').click();
+	expect(await page.getByTestId('knowledge-item').count()).toBe(1);
+	await expect(page.locator('header').getByTitle('Delete knowledge')).not.toBeVisible();
+
+	// Delete the knowledge from the sidebar
+	await page.locator('.section-list-item').first().getByTitle('Delete knowledge').click();
+	await page.getByTitle('Confirm deletion').click();
+	await expect(page.getByText('No knowledge')).toBeVisible();
+	expect(await page.getByTestId('knowledge-item').count()).toBe(0);
+});
+
+test('knowledge cannot be used in a session after deletion', async ({ page }) => {
 	const timestamp = page.getByTestId('knowledge-metadata');
 	const noKnowledgeMessage = page.getByText('No knowledge');
 	const noKnowledgeSelectedMessage = page.getByText('Create new knowlege or choose one from the list');
@@ -86,8 +109,8 @@ test('deletes knowledge', async ({ page }) => {
 	await expect(timestamp).toBeVisible();
 
 	// Delete the knowlege
-	page.on('dialog', dialog => dialog.accept("Are you sure you want to delete this knowledge?"));
-	await page.getByTitle('Delete knowledge').click();
+	await page.locator('header').getByTitle('Delete knowledge').click();
+	await page.getByTitle('Confirm deletion').click();
 	await expect(knowledgeItems).toHaveCount(MOCK_KNOWLEDGE.length - 1);
 	await expect(noKnowledgeSelectedMessage).toBeVisible();
 
diff --git a/tests/session.test.ts b/tests/session.test.ts
index b2443b0..5736e0d 100644
--- a/tests/session.test.ts
+++ b/tests/session.test.ts
@@ -142,11 +142,11 @@ test.describe('Session', () => {
 		expect(await page.getByTestId('session-item').last().textContent()).toContain(MOCK_API_TAGS_RESPONSE.models[0].name);
 
 		// Check the current session is highlighted in the sidebar
-		await expect(page.getByTestId('session-item').first()).toHaveClass(/ section-list-item--active/);
-		await expect(page.getByTestId('session-item').last()).not.toHaveClass(/ section-list-item--active/);
+		await expect(page.locator('.section-list-item').first()).toHaveClass(/ section-list-item--active/);
+		await expect(page.locator('.section-list-item').last()).not.toHaveClass(/ section-list-item--active/);
 	});
 
-	test('deletes a session from the sidebar', async ({ page }) => {
+	test('deletes a session from the header and sidebar', async ({ page }) => {
 		await page.goto('/');
 		await chooseModelFromSettings(page, MOCK_API_TAGS_RESPONSE.models[0].name);
 		await page.getByText('Sessions', { exact: true }).click();
@@ -156,12 +156,28 @@ test.describe('Session', () => {
 		await page.getByTestId('new-session').click();
 		await promptTextarea.fill('Who would win in a fight between Emma Watson and Jessica Alba?');
 		await page.getByText('Run').click();
-		await expect(page.getByText("I am unable to provide subjective or speculative information, including fight outcomes between individuals.")).toBeVisible();
+		await expect(page.getByText(MOCK_SESSION_1_RESPONSE_1.response)).toBeVisible();
+		await expect(page.getByText('No sessions')).not.toBeVisible();
+		expect(await page.getByTestId('session-item').count()).toBe(1);
+
+		// Delete the session from the header
+		await page.locator('header').getByTitle('Delete session').click();
+		await page.getByTitle('Confirm deletion').click();
+		await expect(page.getByText('No sessions')).toBeVisible();
+		expect(await page.getByTestId('session-item').count()).toBe(0);
+
+		await mockCompletionResponse(page, MOCK_SESSION_1_RESPONSE_1);
+		await page.getByTestId('new-session').click();
+		await promptTextarea.fill('Who would win in a fight between Emma Watson and Jessica Alba?');
+		await page.getByText('Run').click();
+		await expect(page.getByText(MOCK_SESSION_1_RESPONSE_1.response)).toBeVisible();
 		await expect(page.getByText('No sessions')).not.toBeVisible();
 		expect(await page.getByTestId('session-item').count()).toBe(1);
 
-		page.on('dialog', dialog => dialog.accept("Are you sure you want to delete this session?"));
-		await page.getByTitle('Delete session').click();
+		// Delete the session from the sidebar
+		await page.locator('.section-list-item').first().hover();
+		await page.locator('.section-list-item').getByTitle('Delete session').click();
+		await page.getByTitle('Confirm deletion').click();
 		await expect(page.getByText('No sessions')).toBeVisible();
 		expect(await page.getByTestId('session-item').count()).toBe(0);
 	});