From a6d73c3ea85a4c7222dd1df811cfc500bef6aa6f Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Tue, 27 Oct 2026 14:53:36 +0100 Subject: [PATCH 1/8] feat: store dashboard bookmarks in the database (#277) Bookmarks are loaded in the (app) layout and changed through the bookmarks API instead of localStorage. Without a signed-in user, or when loading them fails, the bookmark section is hidden. Closes #277 --- e2e/bookmarks.spec.ts | 125 ++++------ e2e/sidebar-bookmarks.spec.ts | 70 ++---- e2e/support/bookmarks.ts | 30 +++ messages/de.json | 3 + messages/en.json | 3 + .../dashboard/AddBookmarkModal.svelte | 90 ++++++-- .../dashboard/AddBookmarkModal.svelte.spec.ts | 54 ++++- .../components/layout/sidebar/Sidebar.svelte | 6 +- src/lib/dashboard/bookmarks-api.test.ts | 67 ++++++ src/lib/dashboard/bookmarks-api.ts | 36 +++ src/lib/dashboard/bookmarks.svelte.spec.ts | 89 -------- src/lib/dashboard/bookmarks.svelte.ts | 55 ----- src/lib/dashboard/types.ts | 11 +- src/routes/(app)/+layout.server.ts | 26 ++- src/routes/(app)/+layout.svelte | 1 + src/routes/(app)/+page.svelte | 215 ++++++++++-------- src/routes/(app)/layout.server.test.ts | 57 +++++ src/routes/(app)/page.svelte.spec.ts | 64 +++++- 18 files changed, 594 insertions(+), 408 deletions(-) create mode 100644 e2e/support/bookmarks.ts create mode 100644 src/lib/dashboard/bookmarks-api.test.ts create mode 100644 src/lib/dashboard/bookmarks-api.ts delete mode 100644 src/lib/dashboard/bookmarks.svelte.spec.ts delete mode 100644 src/lib/dashboard/bookmarks.svelte.ts create mode 100644 src/routes/(app)/layout.server.test.ts diff --git a/e2e/bookmarks.spec.ts b/e2e/bookmarks.spec.ts index 111ffaa2..2c43d027 100644 --- a/e2e/bookmarks.spec.ts +++ b/e2e/bookmarks.spec.ts @@ -1,5 +1,6 @@ import { test, expect } from './support/fixtures.js'; import { waitForHydration } from './support/helpers'; +import { resetBookmarks } from './support/bookmarks.js'; test.describe('Dashboard bookmarks', () => { test.use({ locale: 'en-US' }); @@ -7,9 +8,11 @@ test.describe('Dashboard bookmarks', () => { const addButton = 'Add bookmark'; test.beforeEach(async ({ page }) => { - await page.addInitScript(() => { - localStorage.removeItem('dashboard_bookmarks'); - }); + await resetBookmarks(page); + }); + + test.afterEach(async ({ page }) => { + await resetBookmarks(page); }); test('shows Add bookmark button on dashboard', async ({ page }) => { @@ -118,25 +121,9 @@ test.describe('Dashboard bookmarks', () => { // Names are capped at 40 characters; wide glyphs still overflow the card. const longName = 'W'.repeat(40); const longHost = `${'very-long-subdomain-'.repeat(5)}example.com`; - await page.addInitScript( - ({ name, host }) => { - localStorage.setItem( - 'dashboard_bookmarks', - JSON.stringify([ - { - id: 'b1', - productId: 'superset', - name, - environment: '', - url: `https://${host}/`, - pinned: false, - createdAt: new Date().toISOString() - } - ]) - ); - }, - { name: longName, host: longHost } - ); + await resetBookmarks(page, [ + { productId: 'superset', name: longName, url: `https://${longHost}/` } + ]); await page.goto('/'); await waitForHydration(page); @@ -151,22 +138,14 @@ test.describe('Dashboard bookmarks', () => { }); test('shows the environment once, as a chip, not in the title link', async ({ page }) => { - await page.addInitScript(() => { - localStorage.setItem( - 'dashboard_bookmarks', - JSON.stringify([ - { - id: 'b1', - productId: 'superset', - name: 'Dashboards', - environment: 'staging', - url: 'https://superset.example.com', - pinned: false, - createdAt: new Date().toISOString() - } - ]) - ); - }); + await resetBookmarks(page, [ + { + productId: 'superset', + name: 'Dashboards', + environment: 'staging', + url: 'https://superset.example.com' + } + ]); await page.goto('/'); await waitForHydration(page); @@ -235,7 +214,12 @@ test.describe('Dashboard bookmarks', () => { await expect(imageLink).toHaveAttribute('rel', /external/); }); - test('bookmark persists in localStorage', async ({ page }) => { + test('stores bookmarks on the server, not in the browser', async ({ + page, + browser, + baseURL, + workerStorageState + }) => { await page.goto('/'); await waitForHydration(page); @@ -247,13 +231,17 @@ test.describe('Dashboard bookmarks', () => { await page.getByLabel('URL').fill('https://superset.example.com'); await page.locator('dialog[open]').getByRole('button', { name: addButton }).click(); + await expect(page.getByLabel('Bookmarks').getByText('Dashboards')).toBeVisible(); - const stored = await page.evaluate(() => localStorage.getItem('dashboard_bookmarks')); - expect(stored).toBeTruthy(); - - const bookmarks = JSON.parse(stored!); - expect(bookmarks).toHaveLength(1); - expect(bookmarks[0].name).toBe('Dashboards'); + // A fresh browser context shares only the session cookie, no browser storage. + const context = await browser.newContext({ baseURL, storageState: workerStorageState }); + try { + const other = await context.newPage(); + await other.goto('/'); + await expect(other.getByLabel('Bookmarks').getByText('Dashboards')).toBeVisible(); + } finally { + await context.close(); + } }); test('pins a bookmark from the dashboard with the star button', async ({ page }) => { @@ -275,39 +263,21 @@ test.describe('Dashboard bookmarks', () => { await expect(page.getByRole('button', { name: 'Unpin bookmark Dashboards' })).toBeVisible(); - const stored = await page.evaluate(() => localStorage.getItem('dashboard_bookmarks')); - const bookmarks = JSON.parse(stored!); - expect(bookmarks[0].pinned).toBe(true); - // Unpinning moves the bookmark back to the regular section await page.getByRole('button', { name: 'Unpin bookmark Dashboards' }).click(); - await expect(page.getByRole('button', { name: /pin bookmark/i })).toBeVisible(); - - const storedAfter = await page.evaluate(() => localStorage.getItem('dashboard_bookmarks')); - const bookmarksAfter = JSON.parse(storedAfter!); - expect(bookmarksAfter[0].pinned).toBe(false); + await expect(page.getByRole('button', { name: 'Pin bookmark Dashboards' })).toBeVisible(); }); test('names each card button after its bookmark', async ({ page }) => { - await page.addInitScript(() => { - localStorage.setItem( - 'dashboard_bookmarks', - JSON.stringify( - [ - ['b0f5a7f2-3d1e-4c6b-9a8e-1f2d3c4b5a60', 'superset', 'Dashboards', true], - ['b0f5a7f2-3d1e-4c6b-9a8e-1f2d3c4b5a61', 'airflow', 'Pipelines', false] - ].map(([id, productId, name, pinned]) => ({ - id, - productId, - name, - environment: '', - url: `https://${productId}.example.com`, - pinned, - createdAt: new Date().toISOString() - })) - ) - ); - }); + await resetBookmarks(page, [ + { + productId: 'superset', + name: 'Dashboards', + url: 'https://superset.example.com', + pinned: true + }, + { productId: 'airflow', name: 'Pipelines', url: 'https://airflow.example.com' } + ]); await page.goto('/'); await waitForHydration(page); @@ -337,10 +307,6 @@ test.describe('Dashboard bookmarks', () => { await page.locator('dialog[open]').getByRole('button', { name: addButton }).click(); await expect(page.getByRole('button', { name: 'Unpin bookmark Dashboards' })).toBeVisible(); - - const stored = await page.evaluate(() => localStorage.getItem('dashboard_bookmarks')); - const bookmarks = JSON.parse(stored!); - expect(bookmarks[0].pinned).toBe(true); }); test('pinned state is preserved when editing a bookmark', async ({ page }) => { @@ -393,11 +359,6 @@ test.describe('Dashboard bookmarks', () => { await expect(page.getByLabel('Bookmarks').getByText('Renamed Dashboard')).toBeVisible(); await expect(page.getByLabel('Bookmarks').getByText('Dashboards')).not.toBeVisible(); - - const stored = await page.evaluate(() => localStorage.getItem('dashboard_bookmarks')); - const bookmarks = JSON.parse(stored!); - expect(bookmarks).toHaveLength(1); - expect(bookmarks[0].name).toBe('Renamed Dashboard'); }); test('cancel in edit dialog closes without changes', async ({ page }) => { diff --git a/e2e/sidebar-bookmarks.spec.ts b/e2e/sidebar-bookmarks.spec.ts index 4977f604..016cb855 100644 --- a/e2e/sidebar-bookmarks.spec.ts +++ b/e2e/sidebar-bookmarks.spec.ts @@ -1,11 +1,11 @@ import type { Page } from '@playwright/test'; import { test, expect } from './support/fixtures.js'; import { waitForHydration } from './support/helpers'; +import { resetBookmarks } from './support/bookmarks.js'; test.describe('Sidebar bookmarks', () => { test.use({ locale: 'en-US' }); - const BOOKMARK_ID = 'b6d6c7b6-8f24-4c4a-9c64-4f3b7c19e6a1'; const BOOKMARK_URL = 'https://superset.example.com'; // Bookmarks are also shown on the dashboard, so scope link lookups to the sidebar. @@ -17,36 +17,20 @@ test.describe('Sidebar bookmarks', () => { ); test.beforeEach(async ({ page }) => { - await page.addInitScript( - ({ bookmarkId, bookmarkUrl }) => { - localStorage.removeItem('dashboard_bookmarks'); - localStorage.removeItem('sidebar_tools_open'); - localStorage.setItem( - 'dashboard_bookmarks', - JSON.stringify([ - { - id: bookmarkId, - productId: 'superset', - name: 'Dashboards', - environment: '', - url: bookmarkUrl, - pinned: false, - createdAt: new Date().toISOString() - }, - { - id: 'e7d8c7b6-9f35-4d4b-8d75-5f4c8d2fa7b2', - productId: 'airflow', - name: 'Pipelines', - environment: '', - url: 'https://airflow.example.com', - pinned: true, - createdAt: new Date().toISOString() - } - ]) - ); - }, - { bookmarkId: BOOKMARK_ID, bookmarkUrl: BOOKMARK_URL } - ); + await page.addInitScript(() => localStorage.removeItem('sidebar_tools_open')); + await resetBookmarks(page, [ + { productId: 'superset', name: 'Dashboards', url: BOOKMARK_URL }, + { + productId: 'airflow', + name: 'Pipelines', + url: 'https://airflow.example.com', + pinned: true + } + ]); + }); + + test.afterEach(async ({ page }) => { + await resetBookmarks(page); }); test('shows pinned bookmarks in Favourites and the rest in Tools', async ({ page }) => { @@ -81,30 +65,6 @@ test.describe('Sidebar bookmarks', () => { await expect(trinoLink.locator('..').getByRole('link')).toHaveCount(1); }); - test('ignores invalid bookmarks stored in localStorage', async ({ page }) => { - await page.addInitScript(() => { - localStorage.setItem( - 'dashboard_bookmarks', - JSON.stringify([ - { - id: 'invalid-bookmark', - productId: 'superset', - name: 'Unsafe link', - environment: '', - url: 'javascript:alert(1)', - pinned: false, - createdAt: new Date().toISOString() - } - ]) - ); - }); - - await page.goto('/'); - await waitForHydration(page); - - await expect(page.getByRole('link', { name: 'Unsafe link' })).not.toBeVisible(); - }); - test('tools section is collapsible', async ({ page }) => { await page.goto('/'); await waitForHydration(page); diff --git a/e2e/support/bookmarks.ts b/e2e/support/bookmarks.ts new file mode 100644 index 00000000..0fd16256 --- /dev/null +++ b/e2e/support/bookmarks.ts @@ -0,0 +1,30 @@ +import { expect, type Page } from '@playwright/test'; + +interface BookmarkSeed { + productId: string; + name: string; + url: string; + environment?: string; + pinned?: boolean; +} + +/** + * Replace the signed-in worker user's bookmarks through the API. Bookmarks are + * stored per user and each worker reuses one user for all of its tests, so + * bookmark tests reset them before and after: leftovers would show up in the + * sidebar of every later test in the worker (e.g. a "Dashboards" bookmark next + * to the "Dashboard" nav link). + */ +export async function resetBookmarks(page: Page, seeds: BookmarkSeed[] = []): Promise { + const listed = await page.request.get('/api/bookmarks'); + expect(listed.status()).toBe(200); + for (const { id } of (await listed.json()) as { id: string }[]) { + expect((await page.request.delete(`/api/bookmarks/${id}`)).status()).toBe(204); + } + for (const seed of seeds) { + const created = await page.request.post('/api/bookmarks', { + data: { environment: '', pinned: false, ...seed } + }); + expect(created.status()).toBe(201); + } +} diff --git a/messages/de.json b/messages/de.json index a08e2e08..cad5f07d 100644 --- a/messages/de.json +++ b/messages/de.json @@ -498,6 +498,9 @@ "bookmark_delete_confirm_message": "Möchten Sie \"{name}\" wirklich löschen? Dies kann nicht rückgängig gemacht werden.", "bookmark_delete": "Löschen", "bookmark_cancel": "Abbrechen", + "bookmark_save_error": "Das Lesezeichen konnte nicht gespeichert werden. Bitte versuchen Sie es erneut.", + "bookmark_delete_error": "Das Lesezeichen konnte nicht gelöscht werden. Bitte versuchen Sie es erneut.", + "bookmark_pin_error": "Die Anheftung von „{name}“ konnte nicht geändert werden. Bitte versuchen Sie es erneut.", "bookmark_product_custom": "Eigener Link", "bookmark_default_name_trino": "SQL-Editor", "bookmark_default_name_superset": "Dashboards", diff --git a/messages/en.json b/messages/en.json index 8bd19461..50980fc3 100644 --- a/messages/en.json +++ b/messages/en.json @@ -498,6 +498,9 @@ "bookmark_delete_confirm_message": "Are you sure you want to delete \"{name}\"? This cannot be undone.", "bookmark_delete": "Delete", "bookmark_cancel": "Cancel", + "bookmark_save_error": "The bookmark could not be saved. Please try again.", + "bookmark_delete_error": "The bookmark could not be deleted. Please try again.", + "bookmark_pin_error": "Could not update the pin for \"{name}\". Please try again.", "bookmark_product_custom": "Custom Link", "bookmark_default_name_trino": "SQL Editor", "bookmark_default_name_superset": "Dashboards", diff --git a/src/lib/components/dashboard/AddBookmarkModal.svelte b/src/lib/components/dashboard/AddBookmarkModal.svelte index 4e1f8636..591b5f90 100644 --- a/src/lib/components/dashboard/AddBookmarkModal.svelte +++ b/src/lib/components/dashboard/AddBookmarkModal.svelte @@ -3,10 +3,10 @@ import Modal from '$lib/components/Modal.svelte'; import { getProduct, PRODUCTS, type Product } from '$lib/dashboard/products'; import ProductIcon from './ProductIcon.svelte'; - import { addBookmark, removeBookmark, updateBookmark } from '$lib/dashboard/bookmarks.svelte.js'; + import { addBookmark, removeBookmark, updateBookmark } from '$lib/dashboard/bookmarks-api.js'; import { BOOKMARK_MAX_LENGTH, - bookmarkFormSchema, + bookmarkCreateSchema, bookmarkHostname, type Bookmark } from '$lib/dashboard/types'; @@ -31,6 +31,8 @@ let wasOpen = false; let nameError = $state(false); let urlError = $state(false); + let saving = $state(false); + let requestError = $state(null); let uid = $props.id(); @@ -45,6 +47,7 @@ pinned = target?.pinned ?? false; nameError = false; urlError = false; + requestError = null; } // Only `open` is tracked; the bookmark is read once per open. @@ -72,15 +75,30 @@ } function isValidUrl(value: string): boolean { - return bookmarkFormSchema.shape.url.safeParse(value.trim()).success; + return bookmarkCreateSchema.shape.url.safeParse(value.trim()).success; } function handleUrlBlur() { if (url.trim()) urlError = !isValidUrl(url); } - function handleSubmit(event: SubmitEvent) { + /** Runs a bookmark request; closes the dialog on success, otherwise shows `errorMessage`. */ + async function save(request: () => Promise, errorMessage: string) { + saving = true; + requestError = null; + try { + await request(); + open = false; + } catch { + requestError = errorMessage; + } finally { + saving = false; + } + } + + async function handleSubmit(event: SubmitEvent) { event.preventDefault(); + if (saving) return; const values = { productId: selectedProduct.id, name: name.trim(), @@ -88,7 +106,7 @@ url: url.trim(), pinned }; - const parsed = bookmarkFormSchema.safeParse(values); + const parsed = bookmarkCreateSchema.safeParse(values); if (!parsed.success) { nameError = !values.name; urlError = !isValidUrl(values.url); @@ -96,17 +114,21 @@ return; } - if (bookmark) { - updateBookmark({ ...bookmark, ...parsed.data }); - } else { - addBookmark({ createdAt: new Date().toISOString(), ...parsed.data }); - } - open = false; + const target = bookmark; + await save( + () => (target ? updateBookmark(target.id, parsed.data) : addBookmark(parsed.data)), + m.bookmark_save_error() + ); + } + + function close() { + if (!saving) open = false; } // The confirmation opens on top of the edit dialog, so cancelling (or Escape) // returns to the form with the user's unsaved edits intact. function handleDeleteClick() { + if (saving) return; confirmDeleteOpen = true; } @@ -114,21 +136,25 @@ confirmDeleteOpen = false; } - function handleDeleteConfirm() { - if (!bookmark) return; - removeBookmark(bookmark.id); + async function handleDeleteConfirm() { + const target = bookmark; + if (!target || saving) return; + // Back to the edit dialog, which shows the error if the delete fails. confirmDeleteOpen = false; - open = false; + await save(() => removeBookmark(target.id), m.bookmark_delete_error()); } - + +
@@ -269,16 +295,40 @@
+ {#if requestError} + + {/if} +
{#if isEditing} - {/if} - -
diff --git a/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts b/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts index 389f6946..0bde69c5 100644 --- a/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts +++ b/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts @@ -11,7 +11,7 @@ const { addBookmark, updateBookmark, removeBookmark } = vi.hoisted(() => ({ removeBookmark: vi.fn() })); -vi.mock('$lib/dashboard/bookmarks.svelte.js', () => ({ +vi.mock('$lib/dashboard/bookmarks-api.js', () => ({ addBookmark, updateBookmark, removeBookmark @@ -39,6 +39,9 @@ const pinCheckbox = () => page.getByRole('checkbox', { name: 'Pin bookmark' }); describe('AddBookmarkModal', () => { beforeEach(() => { vi.clearAllMocks(); + addBookmark.mockResolvedValue(undefined); + updateBookmark.mockResolvedValue(undefined); + removeBookmark.mockResolvedValue(undefined); }); it('stores pinned: true when the pin checkbox is checked', async () => { @@ -196,7 +199,54 @@ describe('AddBookmarkModal', () => { await page.getByRole('button', { name: 'Save changes' }).click(); expect(updateBookmark).toHaveBeenCalledWith( - expect.objectContaining({ id: 'b1', name: 'Renamed Dashboard', pinned: true }) + 'b1', + expect.objectContaining({ name: 'Renamed Dashboard', pinned: true }) ); }); + + it('stays open with the entered values and an error when saving fails', async () => { + addBookmark.mockRejectedValue(new Error('500')); + renderModal(); + + await page.getByLabelText('Name').fill('Unsaved Dashboard'); + await page.getByLabelText('URL').fill('https://superset.example.com'); + await page.getByRole('button', { name: 'Add bookmark' }).click(); + + await expect + .element(page.getByRole('alert')) + .toHaveTextContent('The bookmark could not be saved. Please try again.'); + await expect.element(page.getByLabelText('Name')).toHaveValue('Unsaved Dashboard'); + }); + + it('returns to the edit dialog with an error when deleting fails', async () => { + removeBookmark.mockRejectedValue(new Error('500')); + renderModal({ bookmark: savedBookmark }); + + await page.getByRole('button', { name: 'Delete' }).click(); + await page + .getByRole('dialog', { name: 'Delete bookmark?' }) + .getByRole('button', { name: 'Delete' }) + .click(); + + await expect.element(page.getByRole('dialog', { name: 'Edit bookmark' })).toBeVisible(); + await expect + .element(page.getByRole('alert')) + .toHaveTextContent('The bookmark could not be deleted. Please try again.'); + }); + + it('ignores further submits and stays open while a save is in progress', async () => { + addBookmark.mockReturnValue(new Promise(() => {})); + renderModal(); + + await page.getByLabelText('URL').fill('https://superset.example.com'); + await userEvent.keyboard('{Enter}'); + await expect + .element(page.getByRole('button', { name: 'Add bookmark' })) + .toHaveAttribute('aria-disabled', 'true'); + await userEvent.keyboard('{Enter}'); + await userEvent.keyboard('{Escape}'); + + expect(addBookmark).toHaveBeenCalledTimes(1); + await expect.element(page.getByRole('dialog', { name: 'Add bookmark' })).toBeVisible(); + }); }); diff --git a/src/lib/components/layout/sidebar/Sidebar.svelte b/src/lib/components/layout/sidebar/Sidebar.svelte index 3ae277d3..594b6974 100644 --- a/src/lib/components/layout/sidebar/Sidebar.svelte +++ b/src/lib/components/layout/sidebar/Sidebar.svelte @@ -10,7 +10,6 @@ import { MediaQuery } from 'svelte/reactivity'; import type { NavItem } from '$lib/types/navigation.js'; import { getPlatformSection, getToolsSection } from './nav-items.js'; - import { getBookmarks } from '$lib/dashboard/bookmarks.svelte.js'; import { getProduct } from '$lib/dashboard/products'; import ProductIcon from '$lib/components/dashboard/ProductIcon.svelte'; import type { Bookmark } from '$lib/dashboard/types'; @@ -20,17 +19,18 @@ let { collapsed = $bindable(false), mobileOpen = $bindable(false), - storageBrowserEnabled = false + storageBrowserEnabled = false, + bookmarks = [] }: { collapsed?: boolean; mobileOpen?: boolean; storageBrowserEnabled?: boolean; + bookmarks?: Bookmark[]; } = $props(); const platformSection = $derived(getPlatformSection()); const toolsSection = $derived(getToolsSection({ storageBrowserEnabled })); - const bookmarks = $derived(getBookmarks()); const pinnedBookmarks = $derived(bookmarks.filter((b) => b.pinned)); const unpinnedBookmarks = $derived(bookmarks.filter((b) => !b.pinned)); diff --git a/src/lib/dashboard/bookmarks-api.test.ts b/src/lib/dashboard/bookmarks-api.test.ts new file mode 100644 index 00000000..018b5a60 --- /dev/null +++ b/src/lib/dashboard/bookmarks-api.test.ts @@ -0,0 +1,67 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const { invalidate } = vi.hoisted(() => ({ invalidate: vi.fn() })); + +vi.mock('$app/navigation', () => ({ invalidate })); + +import { addBookmark, updateBookmark, removeBookmark } from './bookmarks-api.js'; + +const fetchMock = vi.fn(); +vi.stubGlobal('fetch', fetchMock); + +const FIELDS = { + productId: 'trino', + name: 'Production Trino', + environment: 'prod', + url: 'https://trino.example.com', + pinned: false +}; + +describe('bookmarks-api', () => { + beforeEach(() => { + vi.clearAllMocks(); + fetchMock.mockResolvedValue(new Response(null, { status: 204 })); + invalidate.mockResolvedValue(undefined); + }); + + it('sends each change as JSON to its endpoint and reloads the bookmarks', async () => { + await addBookmark(FIELDS); + await updateBookmark('b/1', { pinned: true }); + await removeBookmark('b/1'); + + expect(fetchMock.mock.calls).toEqual([ + [ + '/api/bookmarks', + { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify(FIELDS) + } + ], + [ + '/api/bookmarks/b%2F1', + { + method: 'PATCH', + headers: { 'content-type': 'application/json' }, + body: '{"pinned":true}' + } + ], + ['/api/bookmarks/b%2F1', { method: 'DELETE' }] + ]); + expect(invalidate).toHaveBeenCalledTimes(3); + expect(invalidate).toHaveBeenCalledWith('app:bookmarks'); + }); + + it('rejects without reloading when the server refuses the change', async () => { + fetchMock.mockResolvedValue(new Response(null, { status: 500 })); + + await expect(removeBookmark('b1')).rejects.toThrow('500'); + expect(invalidate).not.toHaveBeenCalled(); + }); + + it('resolves when the change is saved but reloading the list fails', async () => { + invalidate.mockRejectedValue(new Error('session expired')); + + await expect(addBookmark(FIELDS)).resolves.toBeUndefined(); + }); +}); diff --git a/src/lib/dashboard/bookmarks-api.ts b/src/lib/dashboard/bookmarks-api.ts new file mode 100644 index 00000000..4151e733 --- /dev/null +++ b/src/lib/dashboard/bookmarks-api.ts @@ -0,0 +1,36 @@ +import { invalidate } from '$app/navigation'; +import { BOOKMARKS_DEPENDENCY, type Bookmark, type BookmarkUpdate } from './types'; + +// The bookmarks live in the `(app)` layout data rather than a module-level +// store, which the server would share between users during SSR. Every change +// reloads them from there. + +async function send(path: string, method: string, body?: unknown): Promise { + const response = await fetch(path, { + method, + ...(body === undefined + ? {} + : { headers: { 'content-type': 'application/json' }, body: JSON.stringify(body) }) + }); + if (!response.ok) { + throw new Error(`Bookmark request failed with status ${response.status}`); + } + try { + await invalidate(BOOKMARKS_DEPENDENCY); + } catch { + // The change is saved; reporting it as failed would invite a duplicate. + // The list catches up on the next navigation. + } +} + +export function addBookmark(bookmark: Omit): Promise { + return send('/api/bookmarks', 'POST', bookmark); +} + +export function updateBookmark(id: string, update: BookmarkUpdate): Promise { + return send(`/api/bookmarks/${encodeURIComponent(id)}`, 'PATCH', update); +} + +export function removeBookmark(id: string): Promise { + return send(`/api/bookmarks/${encodeURIComponent(id)}`, 'DELETE'); +} diff --git a/src/lib/dashboard/bookmarks.svelte.spec.ts b/src/lib/dashboard/bookmarks.svelte.spec.ts deleted file mode 100644 index 46937370..00000000 --- a/src/lib/dashboard/bookmarks.svelte.spec.ts +++ /dev/null @@ -1,89 +0,0 @@ -import { afterEach, describe, expect, it, vi } from 'vitest'; - -const LS_KEY = 'dashboard_bookmarks'; - -const validBookmark = { - id: 'b1', - productId: 'superset', - name: 'Dashboards', - environment: '', - url: 'https://superset.example.com', - pinned: false, - createdAt: '2026-01-01T00:00:00.000Z' -}; - -let instance = 0; - -/** - * The store reads localStorage once at module load. A unique query string makes - * Vite evaluate a fresh module instance per test (vi.resetModules() does not in - * browser mode). - */ -async function loadStore(stored: string): Promise { - localStorage.setItem(LS_KEY, stored); - instance += 1; - return import(/* @vite-ignore */ `./bookmarks.svelte.js?instance=${instance}`); -} - -describe('bookmarks store', () => { - afterEach(() => { - localStorage.removeItem(LS_KEY); - vi.restoreAllMocks(); - }); - - it('keeps valid bookmarks when another stored entry is invalid', async () => { - const invalid = { ...validBookmark, id: 'b2', url: 'javascript:alert(1)' }; - const { getBookmarks } = await loadStore(JSON.stringify([invalid, validBookmark])); - - expect(getBookmarks().map((b) => b.id)).toEqual(['b1']); - }); - - it('does not overwrite valid stored bookmarks on the next save', async () => { - const { addBookmark } = await loadStore(JSON.stringify([{ broken: true }, validBookmark])); - - addBookmark({ ...validBookmark, name: 'Pipelines' }); - - const saved = JSON.parse(localStorage.getItem(LS_KEY) ?? '[]'); - expect(saved.map((b: { name: string }) => b.name)).toEqual(['Dashboards', 'Pipelines']); - }); - - it('returns no bookmarks for non-array data', async () => { - const { getBookmarks } = await loadStore(JSON.stringify({ not: 'an array' })); - - expect(getBookmarks()).toEqual([]); - }); - - it('keeps bookmarks added in another tab when saving', async () => { - const { addBookmark, getBookmarks } = await loadStore('[]'); - - // Simulate another tab saving a bookmark. - const otherTab = JSON.stringify([validBookmark]); - localStorage.setItem(LS_KEY, otherTab); - window.dispatchEvent(new StorageEvent('storage', { key: LS_KEY, newValue: otherTab })); - - expect(getBookmarks().map((b) => b.id)).toEqual(['b1']); - - addBookmark({ ...validBookmark, name: 'Pipelines' }); - - const saved = JSON.parse(localStorage.getItem(LS_KEY) ?? '[]'); - expect(saved.map((b: { name: string }) => b.name)).toEqual(['Dashboards', 'Pipelines']); - }); - - it('ignores storage events for other keys', async () => { - const { getBookmarks } = await loadStore(JSON.stringify([validBookmark])); - - window.dispatchEvent(new StorageEvent('storage', { key: 'unrelated', newValue: '[]' })); - - expect(getBookmarks().map((b) => b.id)).toEqual(['b1']); - }); - - it('keeps the bookmark in memory when saving to localStorage fails', async () => { - const { addBookmark, getBookmarks } = await loadStore('[]'); - vi.spyOn(Storage.prototype, 'setItem').mockImplementation(() => { - throw new DOMException('Quota exceeded', 'QuotaExceededError'); - }); - - expect(() => addBookmark(validBookmark)).not.toThrow(); - expect(getBookmarks()).toHaveLength(1); - }); -}); diff --git a/src/lib/dashboard/bookmarks.svelte.ts b/src/lib/dashboard/bookmarks.svelte.ts deleted file mode 100644 index 5872349e..00000000 --- a/src/lib/dashboard/bookmarks.svelte.ts +++ /dev/null @@ -1,55 +0,0 @@ -import { browser } from '$app/environment'; -import { loadFromStorage, persistToStorage } from '$lib/client/local-storage.js'; -import { bookmarkSchema, type Bookmark } from './types'; - -const LS_KEY = 'dashboard_bookmarks'; - -function generateBookmarkId(): string { - return crypto.randomUUID(); -} - -let bookmarks = $state(loadFromStorage(LS_KEY, bookmarkSchema)); - -if (browser) { - // Every write stores the whole list, so pick up changes from other tabs - // before the next write here would overwrite them. A null key means storage - // was cleared. - window.addEventListener('storage', (event) => { - if (event.key === LS_KEY || event.key === null) { - bookmarks = loadFromStorage(LS_KEY, bookmarkSchema); - } - }); -} - -export function getBookmarks(): Bookmark[] { - return bookmarks; -} - -export function addBookmark(bookmark: Omit): void { - bookmarks.push({ ...bookmark, id: generateBookmarkId() }); - persistToStorage(LS_KEY, bookmarks); -} - -export function removeBookmark(id: string): void { - const idx = bookmarks.findIndex((b) => b.id === id); - if (idx !== -1) { - bookmarks.splice(idx, 1); - persistToStorage(LS_KEY, bookmarks); - } -} - -export function updateBookmark(updated: Bookmark): void { - const idx = bookmarks.findIndex((b) => b.id === updated.id); - if (idx !== -1) { - bookmarks[idx] = updated; - persistToStorage(LS_KEY, bookmarks); - } -} - -export function togglePinBookmark(id: string): void { - const idx = bookmarks.findIndex((b) => b.id === id); - if (idx !== -1) { - bookmarks[idx] = { ...bookmarks[idx], pinned: !bookmarks[idx].pinned }; - persistToStorage(LS_KEY, bookmarks); - } -} diff --git a/src/lib/dashboard/types.ts b/src/lib/dashboard/types.ts index 391f2886..60c9f85f 100644 --- a/src/lib/dashboard/types.ts +++ b/src/lib/dashboard/types.ts @@ -1,6 +1,9 @@ import { z } from 'zod'; import { PRODUCT_IDS } from './product-ids'; +/** Dependency of the `(app)` layout load that provides the bookmarks. */ +export const BOOKMARKS_DEPENDENCY = 'app:bookmarks'; + /** Maximum field lengths, enforced by the API and as `maxlength` in the form. */ export const BOOKMARK_MAX_LENGTH = { name: 40, environment: 100, url: 2048 } as const; @@ -15,12 +18,8 @@ export const bookmarkFormSchema = z.object({ pinned: z.boolean() }); -export const bookmarkSchema = bookmarkFormSchema.extend({ - id: z.string(), - createdAt: z.string() -}); - -export type Bookmark = z.infer; +/** A stored bookmark as returned by the API; `createdAt` is an ISO timestamp. */ +export type Bookmark = z.infer & { id: string; createdAt: string }; /** * Body of a bookmark create request. Stored bookmarks keep any product id, so a diff --git a/src/routes/(app)/+layout.server.ts b/src/routes/(app)/+layout.server.ts index a31f7956..b025a0d4 100644 --- a/src/routes/(app)/+layout.server.ts +++ b/src/routes/(app)/+layout.server.ts @@ -1,6 +1,28 @@ import type { LayoutServerLoad } from './$types'; import { storageBrowserEnabled } from '$lib/server/feature-flags.js'; +import { listBookmarks } from '$lib/server/dashboard/bookmarks-db.js'; +import { dashboardBookmarksTotal } from '$lib/server/metrics.js'; +import { BOOKMARKS_DEPENDENCY, type Bookmark } from '$lib/dashboard/types'; -export const load: LayoutServerLoad = async ({ locals }) => { - return { user: locals.user, storageBrowserEnabled }; +export const load: LayoutServerLoad = async ({ locals, depends }) => { + depends(BOOKMARKS_DEPENDENCY); + return { user: locals.user, storageBrowserEnabled, bookmarks: await loadBookmarks(locals) }; }; + +/** + * The signed-in user's bookmarks, or null when bookmarks are unavailable: + * without OIDC there is no user to store them for, and a database failure + * must not take down every page of the app. + */ +async function loadBookmarks(locals: App.Locals): Promise { + if (!locals.user) return null; + try { + const bookmarks = await listBookmarks(locals.user.id); + dashboardBookmarksTotal.inc({ operation: 'load', outcome: 'success' }); + return bookmarks; + } catch (err) { + dashboardBookmarksTotal.inc({ operation: 'load', outcome: 'error' }); + locals.logger.error({ err, user_id: locals.user.id }, 'Failed to load bookmarks'); + return null; + } +} diff --git a/src/routes/(app)/+layout.svelte b/src/routes/(app)/+layout.svelte index 662d6665..69e444ff 100644 --- a/src/routes/(app)/+layout.svelte +++ b/src/routes/(app)/+layout.svelte @@ -74,6 +74,7 @@ bind:collapsed={sidebarCollapsed} bind:mobileOpen storageBrowserEnabled={data.storageBrowserEnabled} + bookmarks={data.bookmarks ?? []} />
diff --git a/src/routes/(app)/+page.svelte b/src/routes/(app)/+page.svelte index 8ca30a6b..ab6624ef 100644 --- a/src/routes/(app)/+page.svelte +++ b/src/routes/(app)/+page.svelte @@ -1,18 +1,25 @@ - +{#if bookmarks} + +{/if}

{m.dashboard_welcome()}

-

{m.dashboard_subtitle()}

+ {#if bookmarks} +

{m.dashboard_subtitle()}

+ {/if}
- + {#if bookmarks} + + {/if}
-
-
-

- {m.bookmark_section_title()} -

- {#if bookmarks.length > 0} - - {m.bookmark_count_links({ count: bookmarks.length })} - - {/if} -
+ {#if bookmarks} +
+
+

+ {m.bookmark_section_title()} +

+ {#if bookmarks.length > 0} + + {m.bookmark_count_links({ count: bookmarks.length })} + + {/if} +
- {#if bookmarks.length > 0} -
- {#each bookmarks as bookmark (bookmark.id)} - {@const product = getProduct(bookmark.productId)} -
- - -
-
- - {bookmark.name} - -
-
- - {product.name} - - {#if bookmark.environment} + +
+
+ + {bookmark.name} + +
+
- {bookmark.environment} + {product.name} - {/if} -
-
- - {bookmarkHostname(bookmark.url)} - + {#if bookmark.environment} + + {bookmark.environment} + + {/if} +
+
+ + {bookmarkHostname(bookmark.url)} + +
+ +
- - -
- {/each} -
- {:else} -
-

{m.dashboard_bookmarks_empty()}

-
- {/if} -
+ {/each} +
+ {:else} +
+

{m.dashboard_bookmarks_empty()}

+
+ {/if} + + {/if}
diff --git a/src/routes/(app)/layout.server.test.ts b/src/routes/(app)/layout.server.test.ts new file mode 100644 index 00000000..7ade1ebc --- /dev/null +++ b/src/routes/(app)/layout.server.test.ts @@ -0,0 +1,57 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const { listBookmarks } = vi.hoisted(() => ({ listBookmarks: vi.fn() })); + +vi.mock('$lib/server/dashboard/bookmarks-db.js', () => ({ listBookmarks })); +vi.mock('$lib/server/feature-flags.js', () => ({ storageBrowserEnabled: false })); + +import { load } from './+layout.server.js'; + +const BOOKMARK = { + id: '00000000-0000-0000-0000-000000000001', + productId: 'trino', + name: 'Production Trino', + environment: 'prod', + url: 'https://trino.example.com', + pinned: false, + createdAt: '2026-10-27T10:00:00.000Z' +}; + +function mockEvent(user: { id: string } | undefined) { + return { + locals: { user, logger: { error: vi.fn() } }, + depends: vi.fn() + } as unknown as Parameters[0]; +} + +describe('(app) layout load', () => { + beforeEach(() => vi.clearAllMocks()); + + it('loads the signed-in user’s bookmarks and can be invalidated', async () => { + listBookmarks.mockResolvedValue([BOOKMARK]); + const event = mockEvent({ id: 'user-1' }); + + const data = await load(event); + + expect(data).toMatchObject({ bookmarks: [BOOKMARK] }); + expect(listBookmarks).toHaveBeenCalledWith('user-1'); + expect(event.depends).toHaveBeenCalledWith('app:bookmarks'); + }); + + it('has no bookmarks without a signed-in user', async () => { + const data = await load(mockEvent(undefined)); + + expect(data).toMatchObject({ bookmarks: null }); + expect(listBookmarks).not.toHaveBeenCalled(); + }); + + it('logs and hides bookmarks instead of failing the page when the database fails', async () => { + listBookmarks.mockRejectedValue(new Error('connection refused')); + const event = mockEvent({ id: 'user-1' }); + + const data = await load(event); + + expect(data).toMatchObject({ bookmarks: null }); + expect(event.locals.logger.error).toHaveBeenCalled(); + }); +}); diff --git a/src/routes/(app)/page.svelte.spec.ts b/src/routes/(app)/page.svelte.spec.ts index 88ad9bbc..561f1072 100644 --- a/src/routes/(app)/page.svelte.spec.ts +++ b/src/routes/(app)/page.svelte.spec.ts @@ -1,11 +1,39 @@ import { page } from 'vitest/browser'; -import { describe, expect, it } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; import { render } from 'vitest-browser-svelte'; +import type { Bookmark } from '$lib/dashboard/types'; import Page from './+page.svelte'; -const renderPage = () => render(Page); +const { updateBookmark, addToast } = vi.hoisted(() => ({ + updateBookmark: vi.fn(), + addToast: vi.fn() +})); + +vi.mock('$lib/dashboard/bookmarks-api.js', () => ({ + addBookmark: vi.fn(), + updateBookmark, + removeBookmark: vi.fn() +})); +vi.mock('$lib/stores/toast.svelte.js', () => ({ addToast })); + +const bookmark: Bookmark = { + id: 'b1', + productId: 'superset', + name: 'Shared Dashboard', + environment: 'prod', + url: 'https://superset.example.com', + pinned: false, + createdAt: '2026-01-01T00:00:00.000Z' +}; + +const renderPage = (bookmarks: Bookmark[] | null = []) => + render(Page, { + data: { user: undefined, storageBrowserEnabled: false, bookmarks } + } as unknown as Parameters>[1]); describe('/(app)/+page.svelte', () => { + beforeEach(() => vi.clearAllMocks()); + it('should render the platform heading', async () => { renderPage(); await expect.element(page.getByRole('heading', { level: 2 })).toBeInTheDocument(); @@ -32,4 +60,36 @@ describe('/(app)/+page.svelte', () => { renderPage(); await expect.element(page.getByText(/no bookmarks yet/i)).toBeInTheDocument(); }); + + it('hides everything bookmark-related when bookmarks are unavailable', async () => { + renderPage(null); + + await expect.element(page.getByRole('heading', { level: 2 })).toBeInTheDocument(); + await expect.element(page.getByRole('heading', { name: 'Bookmarks' })).not.toBeInTheDocument(); + await expect + .element(page.getByRole('button', { name: /add bookmark/i })) + .not.toBeInTheDocument(); + await expect.element(page.getByText(/bookmarks are added by hand/i)).not.toBeInTheDocument(); + }); + + it('toggles the pin through the API', async () => { + updateBookmark.mockResolvedValue(undefined); + renderPage([bookmark]); + + await page.getByRole('button', { name: 'Pin bookmark Shared Dashboard' }).click(); + + expect(updateBookmark).toHaveBeenCalledWith('b1', { pinned: true }); + expect(addToast).not.toHaveBeenCalled(); + }); + + it('shows an error toast when toggling the pin fails', async () => { + updateBookmark.mockRejectedValue(new Error('500')); + renderPage([bookmark]); + + await page.getByRole('button', { name: 'Pin bookmark Shared Dashboard' }).click(); + + await vi.waitFor(() => + expect(addToast).toHaveBeenCalledWith('error', expect.stringContaining('Shared Dashboard')) + ); + }); }); From f4f747afd70da9e3f33656f78fd7cd4b70120a98 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Tue, 27 Oct 2026 14:53:36 +0100 Subject: [PATCH 2/8] refactor: extract dashboard bookmarks into a component (#277) --- .../dashboard/DashboardBookmarks.svelte | 146 ++++++++++++++++ .../DashboardBookmarks.svelte.spec.ts | 95 +++++++++++ src/routes/(app)/+page.svelte | 161 +----------------- src/routes/(app)/page.svelte.spec.ts | 64 +------ 4 files changed, 249 insertions(+), 217 deletions(-) create mode 100644 src/lib/components/dashboard/DashboardBookmarks.svelte create mode 100644 src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts diff --git a/src/lib/components/dashboard/DashboardBookmarks.svelte b/src/lib/components/dashboard/DashboardBookmarks.svelte new file mode 100644 index 00000000..7fb26495 --- /dev/null +++ b/src/lib/components/dashboard/DashboardBookmarks.svelte @@ -0,0 +1,146 @@ + + + + +
+
+
+
+

+ {m.bookmark_section_title()} +

+ {#if bookmarks.length > 0} + + {m.bookmark_count_links({ count: bookmarks.length })} + + {/if} +
+

{m.dashboard_subtitle()}

+
+ +
+ + {#if bookmarks.length > 0} +
+ {#each bookmarks as bookmark (bookmark.id)} + {@const product = getProduct(bookmark.productId)} +
+ + +
+
+ + {bookmark.name} + +
+
+ + {product.name} + + {#if bookmark.environment} + + {bookmark.environment} + + {/if} +
+
+ + {bookmarkHostname(bookmark.url)} + +
+
+ + +
+ {/each} +
+ {:else} +
+

{m.dashboard_bookmarks_empty()}

+
+ {/if} +
diff --git a/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts b/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts new file mode 100644 index 00000000..cf774fbc --- /dev/null +++ b/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts @@ -0,0 +1,95 @@ +import { page } from 'vitest/browser'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { render } from 'vitest-browser-svelte'; +import type { Bookmark } from '$lib/dashboard/types'; +import DashboardBookmarks from './DashboardBookmarks.svelte'; + +const { updateBookmark, addToast } = vi.hoisted(() => ({ + updateBookmark: vi.fn(), + addToast: vi.fn() +})); + +vi.mock('$lib/dashboard/bookmarks-api.js', () => ({ + addBookmark: vi.fn(), + updateBookmark, + removeBookmark: vi.fn() +})); +vi.mock('$lib/stores/toast.svelte.js', () => ({ addToast })); + +const bookmark: Bookmark = { + id: 'b1', + productId: 'superset', + name: 'Shared Dashboard', + environment: 'prod', + url: 'https://superset.example.com', + pinned: false, + createdAt: '2026-01-01T00:00:00.000Z' +}; + +describe('DashboardBookmarks', () => { + beforeEach(() => vi.clearAllMocks()); + + it('renders the section heading, subtitle and Add bookmark button', async () => { + render(DashboardBookmarks, { bookmarks: [] }); + + await expect + .element(page.getByRole('heading', { level: 3, name: 'Bookmarks' })) + .toBeInTheDocument(); + await expect.element(page.getByText(/bookmarks are added by hand/i)).toBeInTheDocument(); + await expect.element(page.getByRole('button', { name: /add bookmark/i })).toBeInTheDocument(); + }); + + it('shows an empty state when there are no bookmarks', async () => { + render(DashboardBookmarks, { bookmarks: [] }); + + await expect.element(page.getByText(/no bookmarks yet/i)).toBeInTheDocument(); + }); + + it('renders a card per bookmark', async () => { + render(DashboardBookmarks, { bookmarks: [bookmark] }); + + await expect + .element(page.getByRole('link', { name: 'Shared Dashboard' })) + .toHaveAttribute('href', 'https://superset.example.com'); + await expect.element(page.getByText('prod')).toBeInTheDocument(); + await expect.element(page.getByText(/no bookmarks yet/i)).not.toBeInTheDocument(); + }); + + it('opens the add dialog', async () => { + render(DashboardBookmarks, { bookmarks: [] }); + + await page.getByRole('button', { name: /add bookmark/i }).click(); + + await expect.element(page.getByRole('dialog', { name: 'Add bookmark' })).toBeVisible(); + }); + + it('opens the edit dialog for a bookmark', async () => { + render(DashboardBookmarks, { bookmarks: [bookmark] }); + + await page.getByRole('button', { name: 'Edit bookmark Shared Dashboard' }).click(); + + await expect.element(page.getByRole('dialog', { name: 'Edit bookmark' })).toBeVisible(); + await expect.element(page.getByLabelText('Name')).toHaveValue('Shared Dashboard'); + }); + + it('toggles the pin through the API', async () => { + updateBookmark.mockResolvedValue(undefined); + render(DashboardBookmarks, { bookmarks: [bookmark] }); + + await page.getByRole('button', { name: 'Pin bookmark Shared Dashboard' }).click(); + + expect(updateBookmark).toHaveBeenCalledWith('b1', { pinned: true }); + expect(addToast).not.toHaveBeenCalled(); + }); + + it('shows an error toast when toggling the pin fails', async () => { + updateBookmark.mockRejectedValue(new Error('500')); + render(DashboardBookmarks, { bookmarks: [bookmark] }); + + await page.getByRole('button', { name: 'Pin bookmark Shared Dashboard' }).click(); + + await vi.waitFor(() => + expect(addToast).toHaveBeenCalledWith('error', expect.stringContaining('Shared Dashboard')) + ); + }); +}); diff --git a/src/routes/(app)/+page.svelte b/src/routes/(app)/+page.svelte index ab6624ef..936cd97f 100644 --- a/src/routes/(app)/+page.svelte +++ b/src/routes/(app)/+page.svelte @@ -1,165 +1,16 @@ -{#if bookmarks} - -{/if} -
-
-
-

{m.dashboard_welcome()}

- {#if bookmarks} -

{m.dashboard_subtitle()}

- {/if} -
- {#if bookmarks} - - {/if} -
- - {#if bookmarks} -
-
-

- {m.bookmark_section_title()} -

- {#if bookmarks.length > 0} - - {m.bookmark_count_links({ count: bookmarks.length })} - - {/if} -
+

{m.dashboard_welcome()}

- {#if bookmarks.length > 0} -
- {#each bookmarks as bookmark (bookmark.id)} - {@const product = getProduct(bookmark.productId)} -
- - -
-
- - {bookmark.name} - -
-
- - {product.name} - - {#if bookmark.environment} - - {bookmark.environment} - - {/if} -
-
- - {bookmarkHostname(bookmark.url)} - -
-
- - -
- {/each} -
- {:else} -
-

{m.dashboard_bookmarks_empty()}

-
- {/if} -
+ + {#if data.bookmarks} + {/if}
diff --git a/src/routes/(app)/page.svelte.spec.ts b/src/routes/(app)/page.svelte.spec.ts index 561f1072..07f4cb5d 100644 --- a/src/routes/(app)/page.svelte.spec.ts +++ b/src/routes/(app)/page.svelte.spec.ts @@ -1,66 +1,27 @@ import { page } from 'vitest/browser'; -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { describe, expect, it } from 'vitest'; import { render } from 'vitest-browser-svelte'; import type { Bookmark } from '$lib/dashboard/types'; import Page from './+page.svelte'; -const { updateBookmark, addToast } = vi.hoisted(() => ({ - updateBookmark: vi.fn(), - addToast: vi.fn() -})); - -vi.mock('$lib/dashboard/bookmarks-api.js', () => ({ - addBookmark: vi.fn(), - updateBookmark, - removeBookmark: vi.fn() -})); -vi.mock('$lib/stores/toast.svelte.js', () => ({ addToast })); - -const bookmark: Bookmark = { - id: 'b1', - productId: 'superset', - name: 'Shared Dashboard', - environment: 'prod', - url: 'https://superset.example.com', - pinned: false, - createdAt: '2026-01-01T00:00:00.000Z' -}; - const renderPage = (bookmarks: Bookmark[] | null = []) => render(Page, { data: { user: undefined, storageBrowserEnabled: false, bookmarks } } as unknown as Parameters>[1]); describe('/(app)/+page.svelte', () => { - beforeEach(() => vi.clearAllMocks()); - it('should render the platform heading', async () => { renderPage(); await expect.element(page.getByRole('heading', { level: 2 })).toBeInTheDocument(); }); - it('should render the dashboard subtitle', async () => { - renderPage(); - await expect.element(page.getByText(/bookmarks are added by hand/i)).toBeInTheDocument(); - }); - - it('should render the Add Bookmark button', async () => { - renderPage(); - await expect.element(page.getByRole('button', { name: /add bookmark/i })).toBeInTheDocument(); - }); - - it('should render the bookmarks section heading', async () => { + it('should render the bookmarks section', async () => { renderPage(); await expect .element(page.getByRole('heading', { level: 3, name: 'Bookmarks' })) .toBeInTheDocument(); }); - it('should show an empty state when there are no bookmarks', async () => { - renderPage(); - await expect.element(page.getByText(/no bookmarks yet/i)).toBeInTheDocument(); - }); - it('hides everything bookmark-related when bookmarks are unavailable', async () => { renderPage(null); @@ -71,25 +32,4 @@ describe('/(app)/+page.svelte', () => { .not.toBeInTheDocument(); await expect.element(page.getByText(/bookmarks are added by hand/i)).not.toBeInTheDocument(); }); - - it('toggles the pin through the API', async () => { - updateBookmark.mockResolvedValue(undefined); - renderPage([bookmark]); - - await page.getByRole('button', { name: 'Pin bookmark Shared Dashboard' }).click(); - - expect(updateBookmark).toHaveBeenCalledWith('b1', { pinned: true }); - expect(addToast).not.toHaveBeenCalled(); - }); - - it('shows an error toast when toggling the pin fails', async () => { - updateBookmark.mockRejectedValue(new Error('500')); - renderPage([bookmark]); - - await page.getByRole('button', { name: 'Pin bookmark Shared Dashboard' }).click(); - - await vi.waitFor(() => - expect(addToast).toHaveBeenCalledWith('error', expect.stringContaining('Shared Dashboard')) - ); - }); }); From df105eaea9668958ee2c71b86cb07ab76c6e044c Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Tue, 27 Oct 2026 15:01:15 +0100 Subject: [PATCH 3/8] test: move bookmark UI checks from E2E to component tests --- e2e/bookmarks.spec.ts | 371 +++--------------- .../dashboard/AddBookmarkModal.svelte.spec.ts | 74 ++++ .../DashboardBookmarks.svelte.spec.ts | 60 ++- 3 files changed, 174 insertions(+), 331 deletions(-) diff --git a/e2e/bookmarks.spec.ts b/e2e/bookmarks.spec.ts index 2c43d027..61260a00 100644 --- a/e2e/bookmarks.spec.ts +++ b/e2e/bookmarks.spec.ts @@ -2,6 +2,9 @@ import { test, expect } from './support/fixtures.js'; import { waitForHydration } from './support/helpers'; import { resetBookmarks } from './support/bookmarks.js'; +// Form validation, dialog behaviour and card rendering are covered by the +// AddBookmarkModal and DashboardBookmarks component tests. These tests cover +// the flows through the server and the real page layout. test.describe('Dashboard bookmarks', () => { test.use({ locale: 'en-US' }); @@ -15,89 +18,37 @@ test.describe('Dashboard bookmarks', () => { await resetBookmarks(page); }); - test('shows Add bookmark button on dashboard', async ({ page }) => { - await page.goto('/'); - await waitForHydration(page); - - await expect(page.getByRole('button', { name: addButton })).toBeVisible(); - }); - - test('opens modal when Add bookmark is clicked', async ({ page }) => { - await page.goto('/'); - await waitForHydration(page); - - await page.getByRole('button', { name: addButton }).click(); - - await expect(page.locator('dialog[open]')).toBeVisible(); - }); - - test('modal shows product selection and form fields', async ({ page }) => { + test('adds a bookmark and stores it on the server, not in the browser', async ({ + page, + browser, + baseURL, + workerStorageState + }) => { await page.goto('/'); await waitForHydration(page); await page.getByRole('button', { name: addButton }).click(); - - await expect(page.locator('dialog[open]')).toBeVisible(); - - // Product buttons are visible (names are inside buttons with logos/initials) - await expect(page.locator('button[aria-pressed]').filter({ hasText: 'Trino' })).toBeVisible(); - await expect( - page.locator('button[aria-pressed]').filter({ hasText: 'Superset' }) - ).toBeVisible(); - - // Bookmarks always open externally; no launch mode is configurable. - await expect(page.getByText('Inside Cockpit')).not.toBeVisible(); - await expect(page.getByText('New Tab')).not.toBeVisible(); - - // Form fields - await expect(page.getByLabel('Name')).toBeVisible(); - await expect(page.getByLabel(/Environment/)).toBeVisible(); - await expect(page.getByLabel('URL')).toBeVisible(); - - // Pinned checkbox label - await expect(page.getByRole('checkbox', { name: 'Pin bookmark' })).toBeVisible(); - - // Preview section - await expect(page.getByText('Preview')).toBeVisible(); - }); - - test('selecting a product fills the default name', async ({ page }) => { - await page.goto('/'); - await waitForHydration(page); - - await page.getByRole('button', { name: 'Add bookmark' }).click(); await expect(page.locator('dialog[open]')).toBeVisible(); await page.locator('button[aria-pressed]').filter({ hasText: 'Superset' }).click(); - - await expect(page.getByLabel('Name')).toHaveValue('Dashboards'); - }); - - test('preview updates as user fills the form', async ({ page }) => { - await page.goto('/'); - await waitForHydration(page); - - await page.getByRole('button', { name: 'Add bookmark' }).click(); - await expect(page.locator('dialog[open]')).toBeVisible(); - - await page.getByLabel('Name').fill('My Dashboard'); + await page.getByLabel('Name').fill('Dashboards'); await page.getByLabel('URL').fill('https://superset.example.com'); - await expect(page.getByText('superset.example.com')).toBeVisible(); - }); - - test('rejects non-HTTP bookmark URLs', async ({ page }) => { - await page.goto('/'); - await waitForHydration(page); - - await page.getByRole('button', { name: addButton }).click(); - await page.getByLabel('Name').fill('Unsafe link'); - await page.getByLabel('URL').fill('javascript:alert(1)'); await page.locator('dialog[open]').getByRole('button', { name: addButton }).click(); - await expect(page.locator('dialog[open]')).toBeVisible(); - await expect(page.getByText('Enter a valid HTTP or HTTPS URL.')).toBeVisible(); - await expect(page.getByLabel('URL')).toHaveAttribute('aria-invalid', 'true'); + await expect(page.locator('dialog[open]')).toHaveCount(0); + await expect(page.getByLabel('Bookmarks').getByText('Dashboards')).toBeVisible(); + await expect(page.getByText('superset.example.com')).toBeVisible(); + + // A fresh browser context shares only the session cookie, no browser storage. + const context = await browser.newContext({ baseURL, storageState: workerStorageState }); + try { + const other = await context.newPage(); + await other.goto('/'); + await expect(other.getByLabel('Bookmarks').getByText('Dashboards')).toBeVisible(); + } finally { + await context.close(); + } }); test('accepts internal URLs such as IP addresses and service names', async ({ page }) => { @@ -137,299 +88,65 @@ test.describe('Dashboard bookmarks', () => { expect(hostBox!.x + hostBox!.width).toBeLessThanOrEqual(starBox!.x); }); - test('shows the environment once, as a chip, not in the title link', async ({ page }) => { - await resetBookmarks(page, [ - { - productId: 'superset', - name: 'Dashboards', - environment: 'staging', - url: 'https://superset.example.com' - } - ]); - - await page.goto('/'); - await waitForHydration(page); - - const section = page.getByLabel('Bookmarks'); - await expect(section.getByRole('link', { name: 'Dashboards', exact: true })).toBeVisible(); - await expect(section.getByText('staging', { exact: true })).toHaveCount(1); - }); - - test('prefills the default name and shows an error when it is cleared', async ({ page }) => { - await page.goto('/'); - await waitForHydration(page); - - await page.getByRole('button', { name: addButton }).click(); - await expect(page.getByLabel('Name')).toHaveValue('SQL Editor'); - - await page.getByLabel('Name').fill(''); - await page.getByLabel('URL').fill('https://trino.example.com'); - await page.locator('dialog[open]').getByRole('button', { name: addButton }).click(); - - await expect(page.getByText('Enter a name.')).toBeVisible(); - await expect(page.getByLabel('Name')).toBeFocused(); - }); - - test('adds a bookmark and shows it on the dashboard', async ({ page }) => { - await page.goto('/'); - await waitForHydration(page); - - await page.getByRole('button', { name: 'Add bookmark' }).click(); - await expect(page.locator('dialog[open]')).toBeVisible(); - - await page.locator('button[aria-pressed]').filter({ hasText: 'Superset' }).click(); - await page.getByLabel('Name').fill('Dashboards'); - await page.getByLabel('URL').fill('https://superset.example.com'); - - await page.locator('dialog[open]').getByRole('button', { name: 'Add bookmark' }).click(); - - // Bookmark section is visible - await expect(page.getByRole('heading', { name: 'Bookmarks' })).toBeVisible(); - await expect(page.getByLabel('Bookmarks').getByText('Dashboards')).toBeVisible(); - await expect(page.getByText('superset.example.com')).toBeVisible(); - }); - - test('opens dashboard bookmark images and titles in a new tab', async ({ page }) => { - await page.goto('/'); - await waitForHydration(page); - - await page.getByRole('button', { name: addButton }).click(); - await page.getByLabel('Name').fill('Dashboards'); - await page.getByLabel('URL').fill('https://superset.example.com'); - await page.locator('dialog[open]').getByRole('button', { name: addButton }).click(); - - const bookmarkSection = page.getByLabel('Bookmarks'); - const titleLink = bookmarkSection.getByRole('link', { name: 'Dashboards' }); - // The image link duplicates the title link and is hidden from assistive technology. - await expect(bookmarkSection.getByRole('link', { name: 'Trino' })).toHaveCount(0); - const imageLink = bookmarkSection.locator('a[aria-hidden="true"]'); - - await expect(titleLink).toHaveAttribute('href', 'https://superset.example.com'); - await expect(titleLink).toHaveAttribute('target', '_blank'); - await expect(titleLink).toHaveAttribute('rel', /noopener/); - await expect(titleLink).toHaveAttribute('rel', /external/); - await expect(imageLink).toHaveAttribute('href', 'https://superset.example.com'); - await expect(imageLink).toHaveAttribute('target', '_blank'); - await expect(imageLink).toHaveAttribute('rel', /noopener/); - await expect(imageLink).toHaveAttribute('rel', /external/); - }); - - test('stores bookmarks on the server, not in the browser', async ({ - page, - browser, - baseURL, - workerStorageState - }) => { - await page.goto('/'); - await waitForHydration(page); - - await page.getByRole('button', { name: addButton }).click(); - await expect(page.locator('dialog[open]')).toBeVisible(); - - await page.locator('button[aria-pressed]').filter({ hasText: 'Superset' }).click(); - await page.getByLabel('Name').fill('Dashboards'); - await page.getByLabel('URL').fill('https://superset.example.com'); - - await page.locator('dialog[open]').getByRole('button', { name: addButton }).click(); - await expect(page.getByLabel('Bookmarks').getByText('Dashboards')).toBeVisible(); - - // A fresh browser context shares only the session cookie, no browser storage. - const context = await browser.newContext({ baseURL, storageState: workerStorageState }); - try { - const other = await context.newPage(); - await other.goto('/'); - await expect(other.getByLabel('Bookmarks').getByText('Dashboards')).toBeVisible(); - } finally { - await context.close(); - } - }); - - test('pins a bookmark from the dashboard with the star button', async ({ page }) => { + test('pins via the dialog checkbox and unpins with the star button', async ({ page }) => { await page.goto('/'); await waitForHydration(page); await page.getByRole('button', { name: addButton }).click(); - await expect(page.locator('dialog[open]')).toBeVisible(); - - await page.getByLabel('Name').fill('Dashboards'); - await page.getByLabel('URL').fill('https://superset.example.com'); - - await page.locator('dialog[open]').getByRole('button', { name: addButton }).click(); - - // Bookmark appears in the regular section unpinned. - await expect(page.getByLabel('Bookmarks').getByText('Dashboards')).toBeVisible(); - - await page.getByRole('button', { name: /pin bookmark/i }).click(); - - await expect(page.getByRole('button', { name: 'Unpin bookmark Dashboards' })).toBeVisible(); - - // Unpinning moves the bookmark back to the regular section - await page.getByRole('button', { name: 'Unpin bookmark Dashboards' }).click(); - await expect(page.getByRole('button', { name: 'Pin bookmark Dashboards' })).toBeVisible(); - }); - - test('names each card button after its bookmark', async ({ page }) => { - await resetBookmarks(page, [ - { - productId: 'superset', - name: 'Dashboards', - url: 'https://superset.example.com', - pinned: true - }, - { productId: 'airflow', name: 'Pipelines', url: 'https://airflow.example.com' } - ]); - - await page.goto('/'); - await waitForHydration(page); - - const section = page.getByLabel('Bookmarks'); - for (const label of [ - 'Edit bookmark Dashboards', - 'Unpin bookmark Dashboards', - 'Edit bookmark Pipelines', - 'Pin bookmark Pipelines' - ]) { - await expect(section.getByRole('button', { name: label, exact: true })).toBeVisible(); - } - }); - - test('pins a bookmark via the checkbox in the modal', async ({ page }) => { - await page.goto('/'); - await waitForHydration(page); - - await page.getByRole('button', { name: addButton }).click(); - await expect(page.locator('dialog[open]')).toBeVisible(); - await page.getByLabel('Name').fill('Dashboards'); await page.getByLabel('URL').fill('https://superset.example.com'); await page.locator('dialog[open]').getByRole('checkbox', { name: 'Pin bookmark' }).click(); - await page.locator('dialog[open]').getByRole('button', { name: addButton }).click(); - await expect(page.getByRole('button', { name: 'Unpin bookmark Dashboards' })).toBeVisible(); - }); - - test('pinned state is preserved when editing a bookmark', async ({ page }) => { - await page.goto('/'); - await waitForHydration(page); - - await page.getByRole('button', { name: addButton }).click(); - await expect(page.locator('dialog[open]')).toBeVisible(); - - await page.getByLabel('Name').fill('Dashboards'); - await page.getByLabel('URL').fill('https://superset.example.com'); - await page.locator('dialog[open]').getByRole('checkbox', { name: 'Pin bookmark' }).click(); - - await page.locator('dialog[open]').getByRole('button', { name: addButton }).click(); + const unpin = page.getByRole('button', { name: 'Unpin bookmark Dashboards' }); + const pin = page.getByRole('button', { name: 'Pin bookmark Dashboards' }); + await expect(unpin).toBeVisible(); + // The edit dialog shows the stored pinned state. await page.getByRole('button', { name: 'Edit bookmark Dashboards' }).click(); - await expect(page.locator('dialog[open]')).toBeVisible(); - await expect( page.locator('dialog[open]').getByRole('checkbox', { name: 'Pin bookmark' }) ).toBeChecked(); + await page.locator('dialog[open]').getByRole('button', { name: 'Cancel' }).click(); + + await unpin.click(); + await expect(pin).toBeVisible(); + await pin.click(); + await expect(unpin).toBeVisible(); }); test('edits a bookmark', async ({ page }) => { + await resetBookmarks(page, [ + { productId: 'superset', name: 'Dashboards', url: 'https://superset.example.com' } + ]); await page.goto('/'); await waitForHydration(page); - await page.getByRole('button', { name: 'Add bookmark' }).click(); - await expect(page.locator('dialog[open]')).toBeVisible(); - - await page.locator('button[aria-pressed]').filter({ hasText: 'Superset' }).click(); - await page.getByLabel('Name').fill('Dashboards'); - await page.getByLabel('URL').fill('https://superset.example.com'); - - await page.locator('dialog[open]').getByRole('button', { name: 'Add bookmark' }).click(); - await page.getByRole('button', { name: 'Edit bookmark Dashboards' }).click(); - await expect(page.locator('dialog[open]')).toBeVisible(); - - // Modal opens prefilled and shows edit title - await expect( - page.locator('dialog[open]').getByRole('heading', { name: 'Edit bookmark' }) - ).toBeVisible(); - await expect(page.getByLabel('Name')).toHaveValue('Dashboards'); - await expect(page.getByLabel('URL')).toHaveValue('https://superset.example.com'); - await page.getByLabel('Name').fill('Renamed Dashboard'); - await page.locator('dialog[open]').getByRole('button', { name: 'Save changes' }).click(); + await expect(page.locator('dialog[open]')).toHaveCount(0); await expect(page.getByLabel('Bookmarks').getByText('Renamed Dashboard')).toBeVisible(); await expect(page.getByLabel('Bookmarks').getByText('Dashboards')).not.toBeVisible(); }); - test('cancel in edit dialog closes without changes', async ({ page }) => { - await page.goto('/'); - await waitForHydration(page); - - await page.getByRole('button', { name: 'Add bookmark' }).click(); - await expect(page.locator('dialog[open]')).toBeVisible(); - - await page.locator('button[aria-pressed]').filter({ hasText: 'Superset' }).click(); - await page.getByLabel('Name').fill('Dashboards'); - await page.getByLabel('URL').fill('https://superset.example.com'); - - await page.locator('dialog[open]').getByRole('button', { name: 'Add bookmark' }).click(); - - await page.getByRole('button', { name: 'Edit bookmark Dashboards' }).click(); - await expect(page.locator('dialog[open]')).toBeVisible(); - - await page.getByLabel('Name').fill('Not Saved'); - await page.locator('dialog[open]').getByRole('button', { name: 'Cancel' }).click(); - - await expect(page.locator('dialog[open]')).not.toBeVisible(); - await expect(page.getByLabel('Bookmarks').getByText('Dashboards')).toBeVisible(); - await expect(page.getByLabel('Bookmarks').getByText('Not Saved')).not.toBeVisible(); - - // Reopening the edit modal prefills the bookmark values again - await page.getByRole('button', { name: 'Edit bookmark Dashboards' }).click(); - await expect(page.locator('dialog[open]')).toBeVisible(); - await expect(page.getByLabel('Name')).toHaveValue('Dashboards'); - await expect(page.getByLabel('URL')).toHaveValue('https://superset.example.com'); - }); - - test('deletes a bookmark from the edit modal with confirmation', async ({ page }) => { + test('deletes a bookmark from the edit dialog after confirmation', async ({ page }) => { + await resetBookmarks(page, [ + { productId: 'superset', name: 'Dashboards', url: 'https://superset.example.com' } + ]); await page.goto('/'); await waitForHydration(page); - await page.getByRole('button', { name: 'Add bookmark' }).click(); - await expect(page.locator('dialog[open]')).toBeVisible(); - - await page.locator('button[aria-pressed]').filter({ hasText: 'Superset' }).click(); - await page.getByLabel('Name').fill('Dashboards'); - await page.getByLabel('URL').fill('https://superset.example.com'); - - await page.locator('dialog[open]').getByRole('button', { name: 'Add bookmark' }).click(); - await page.getByRole('button', { name: 'Edit bookmark Dashboards' }).click(); const editDialog = page.getByRole('dialog', { name: 'Edit bookmark' }); const confirmDialog = page.getByRole('dialog', { name: 'Delete bookmark?' }); - await expect(editDialog).toBeVisible(); - - // Unsaved edits survive cancelling the confirmation, by button and by Escape - await editDialog.getByLabel('Name').fill('Renamed but not saved'); - await editDialog.getByRole('button', { name: 'Delete' }).click(); - await expect(confirmDialog).toBeVisible(); - await confirmDialog.getByRole('button', { name: 'Cancel' }).click(); - await expect(confirmDialog).toBeHidden(); - await expect(editDialog.getByLabel('Name')).toHaveValue('Renamed but not saved'); - - await editDialog.getByRole('button', { name: 'Delete' }).click(); - await expect(confirmDialog).toBeVisible(); - await page.keyboard.press('Escape'); - await expect(confirmDialog).toBeHidden(); - await expect(editDialog).toBeVisible(); - await expect(editDialog.getByLabel('Name')).toHaveValue('Renamed but not saved'); - - // Delete again and confirm: both dialogs close and the bookmark is gone await editDialog.getByRole('button', { name: 'Delete' }).click(); await confirmDialog.getByRole('button', { name: 'Delete' }).click(); + await expect(confirmDialog).toBeHidden(); await expect(editDialog).toBeHidden(); - await expect(page.getByLabel('Bookmarks').getByText('Dashboards')).not.toBeVisible(); + await expect(page.getByText(/no bookmarks yet/i)).toBeVisible(); }); }); diff --git a/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts b/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts index 0bde69c5..cba8b91e 100644 --- a/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts +++ b/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts @@ -101,6 +101,54 @@ describe('AddBookmarkModal', () => { expect(addBookmark).not.toHaveBeenCalled(); }); + it('rejects non-HTTP URLs', async () => { + renderModal(); + + await page.getByLabelText('Name').fill('Unsafe link'); + await page.getByLabelText('URL').fill('javascript:alert(1)'); + await page.getByRole('button', { name: 'Add bookmark' }).click(); + + await expect.element(page.getByText('Enter a valid HTTP or HTTPS URL.')).toBeVisible(); + await expect.element(page.getByLabelText('URL')).toHaveAttribute('aria-invalid', 'true'); + expect(addBookmark).not.toHaveBeenCalled(); + }); + + it('shows an error and focuses the name field when the name is cleared', async () => { + renderModal(); + + await page.getByLabelText('Name').fill(''); + await page.getByLabelText('URL').fill('https://trino.example.com'); + await page.getByRole('button', { name: 'Add bookmark' }).click(); + + await expect.element(page.getByText('Enter a name.')).toBeVisible(); + await expect.element(page.getByLabelText('Name')).toHaveFocus(); + expect(addBookmark).not.toHaveBeenCalled(); + }); + + it('shows the product choices, the form fields and the preview', async () => { + renderModal(); + + await expect.element(page.getByRole('button', { name: 'Trino' })).toBeVisible(); + await expect.element(page.getByRole('button', { name: 'Superset' })).toBeVisible(); + await expect.element(page.getByLabelText('Name')).toBeVisible(); + await expect.element(page.getByLabelText(/Environment/)).toBeVisible(); + await expect.element(page.getByLabelText('URL')).toBeVisible(); + await expect.element(pinCheckbox()).toBeVisible(); + await expect.element(page.getByText('Preview')).toBeVisible(); + }); + + it('shows the entered name, environment and hostname in the preview', async () => { + renderModal(); + + await page.getByLabelText('Name').fill('My Dashboard'); + await page.getByLabelText(/Environment/).fill('staging'); + await page.getByLabelText('URL').fill('https://superset.example.com/dashboards'); + + await expect.element(page.getByText('My Dashboard')).toBeVisible(); + await expect.element(page.getByText('staging', { exact: true })).toBeVisible(); + await expect.element(page.getByText('superset.example.com', { exact: true })).toBeVisible(); + }); + it('shows the URL error when leaving the field with a malformed URL', async () => { renderModal(); @@ -116,6 +164,17 @@ describe('AddBookmarkModal', () => { await expect.element(page.getByLabelText('Name')).toHaveValue('SQL Editor'); }); + it('replaces the default name when another product is selected', async () => { + renderModal(); + + await page.getByRole('button', { name: 'Superset' }).click(); + + await expect.element(page.getByLabelText('Name')).toHaveValue('Dashboards'); + await expect + .element(page.getByRole('button', { name: 'Superset' })) + .toHaveAttribute('aria-pressed', 'true'); + }); + it('refills the default name on product switch after the name was cleared', async () => { renderModal(); @@ -178,6 +237,21 @@ describe('AddBookmarkModal', () => { expect(removeBookmark).not.toHaveBeenCalled(); }); + it('keeps unsaved edits when the delete confirmation is dismissed with Escape', async () => { + renderModal({ bookmark: savedBookmark }); + const editDialog = page.getByRole('dialog', { name: 'Edit bookmark' }); + + await editDialog.getByLabelText('Name').fill('Renamed but not saved'); + await editDialog.getByRole('button', { name: 'Delete' }).click(); + const confirmDialog = page.getByRole('dialog', { name: 'Delete bookmark?' }); + await expect.element(confirmDialog).toBeVisible(); + await userEvent.keyboard('{Escape}'); + + await expect.element(confirmDialog).not.toBeInTheDocument(); + await expect.element(editDialog.getByLabelText('Name')).toHaveValue('Renamed but not saved'); + expect(removeBookmark).not.toHaveBeenCalled(); + }); + it('removes the bookmark and closes both dialogs when deletion is confirmed', async () => { renderModal({ bookmark: savedBookmark }); diff --git a/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts b/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts index cf774fbc..cb8b9808 100644 --- a/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts +++ b/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts @@ -45,16 +45,53 @@ describe('DashboardBookmarks', () => { await expect.element(page.getByText(/no bookmarks yet/i)).toBeInTheDocument(); }); - it('renders a card per bookmark', async () => { + it('renders a card per bookmark with the environment once, as a chip', async () => { render(DashboardBookmarks, { bookmarks: [bookmark] }); await expect - .element(page.getByRole('link', { name: 'Shared Dashboard' })) - .toHaveAttribute('href', 'https://superset.example.com'); - await expect.element(page.getByText('prod')).toBeInTheDocument(); + .element(page.getByRole('link', { name: 'Shared Dashboard', exact: true })) + .toBeVisible(); + expect(page.getByText('prod', { exact: true }).elements()).toHaveLength(1); + await expect.element(page.getByText('superset.example.com', { exact: true })).toBeVisible(); await expect.element(page.getByText(/no bookmarks yet/i)).not.toBeInTheDocument(); }); + it('opens the bookmark image and title in a new tab', async () => { + const { container } = render(DashboardBookmarks, { bookmarks: [bookmark] }); + + // The image link duplicates the title link and is hidden from assistive technology. + const imageLink = container.querySelector('a[aria-hidden="true"]')!; + expect(imageLink.getAttribute('tabindex')).toBe('-1'); + for (const link of [ + page.getByRole('link', { name: 'Shared Dashboard' }).element(), + imageLink + ]) { + expect(link.getAttribute('href')).toBe('https://superset.example.com'); + expect(link.getAttribute('target')).toBe('_blank'); + expect(link.getAttribute('rel')).toContain('noopener'); + expect(link.getAttribute('rel')).toContain('external'); + } + expect(page.getByRole('link').elements()).toHaveLength(1); + }); + + it('names each card button after its bookmark', async () => { + render(DashboardBookmarks, { + bookmarks: [ + { ...bookmark, name: 'Dashboards', pinned: true }, + { ...bookmark, id: 'b2', productId: 'airflow', name: 'Pipelines' } + ] + }); + + for (const label of [ + 'Edit bookmark Dashboards', + 'Unpin bookmark Dashboards', + 'Edit bookmark Pipelines', + 'Pin bookmark Pipelines' + ]) { + await expect.element(page.getByRole('button', { name: label, exact: true })).toBeVisible(); + } + }); + it('opens the add dialog', async () => { render(DashboardBookmarks, { bookmarks: [] }); @@ -72,6 +109,21 @@ describe('DashboardBookmarks', () => { await expect.element(page.getByLabelText('Name')).toHaveValue('Shared Dashboard'); }); + it('discards cancelled edits and prefills the saved values again on reopen', async () => { + render(DashboardBookmarks, { bookmarks: [bookmark] }); + const editButton = page.getByRole('button', { name: 'Edit bookmark Shared Dashboard' }); + + await editButton.click(); + await page.getByLabelText('Name').fill('Not Saved'); + await page.getByRole('button', { name: 'Cancel' }).click(); + await expect.element(page.getByRole('dialog')).not.toBeInTheDocument(); + await editButton.click(); + + await expect.element(page.getByLabelText('Name')).toHaveValue('Shared Dashboard'); + await expect.element(page.getByLabelText('URL')).toHaveValue('https://superset.example.com'); + expect(updateBookmark).not.toHaveBeenCalled(); + }); + it('toggles the pin through the API', async () => { updateBookmark.mockResolvedValue(undefined); render(DashboardBookmarks, { bookmarks: [bookmark] }); From 7fa93dfd76a8c90088170e3bb2973fe7c1ff9fce Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Tue, 27 Oct 2026 16:31:33 +0100 Subject: [PATCH 4/8] feat: show a message when bookmarks cannot be loaded (#277) The layout load now tells a failed load apart from having no signed-in user. Without a user the section stays hidden; after a failure it says the bookmarks could not be loaded. --- messages/de.json | 1 + messages/en.json | 1 + .../dashboard/DashboardBookmarks.svelte | 31 +++++++++++++------ .../DashboardBookmarks.svelte.spec.ts | 15 +++++++++ src/routes/(app)/+layout.server.ts | 18 ++++++----- src/routes/(app)/+page.svelte | 6 ++-- src/routes/(app)/layout.server.test.ts | 8 ++--- src/routes/(app)/page.svelte.spec.ts | 17 ++++++++-- 8 files changed, 70 insertions(+), 27 deletions(-) diff --git a/messages/de.json b/messages/de.json index cad5f07d..d64654e3 100644 --- a/messages/de.json +++ b/messages/de.json @@ -501,6 +501,7 @@ "bookmark_save_error": "Das Lesezeichen konnte nicht gespeichert werden. Bitte versuchen Sie es erneut.", "bookmark_delete_error": "Das Lesezeichen konnte nicht gelöscht werden. Bitte versuchen Sie es erneut.", "bookmark_pin_error": "Die Anheftung von „{name}“ konnte nicht geändert werden. Bitte versuchen Sie es erneut.", + "bookmark_load_error": "Die Lesezeichen konnten nicht geladen werden. Bitte laden Sie die Seite später neu.", "bookmark_product_custom": "Eigener Link", "bookmark_default_name_trino": "SQL-Editor", "bookmark_default_name_superset": "Dashboards", diff --git a/messages/en.json b/messages/en.json index 50980fc3..b8f70309 100644 --- a/messages/en.json +++ b/messages/en.json @@ -501,6 +501,7 @@ "bookmark_save_error": "The bookmark could not be saved. Please try again.", "bookmark_delete_error": "The bookmark could not be deleted. Please try again.", "bookmark_pin_error": "Could not update the pin for \"{name}\". Please try again.", + "bookmark_load_error": "Bookmarks could not be loaded. Please reload the page later.", "bookmark_product_custom": "Custom Link", "bookmark_default_name_trino": "SQL Editor", "bookmark_default_name_superset": "Dashboards", diff --git a/src/lib/components/dashboard/DashboardBookmarks.svelte b/src/lib/components/dashboard/DashboardBookmarks.svelte index 7fb26495..b8d1a37a 100644 --- a/src/lib/components/dashboard/DashboardBookmarks.svelte +++ b/src/lib/components/dashboard/DashboardBookmarks.svelte @@ -12,7 +12,12 @@ import ProductIcon from '$lib/components/dashboard/ProductIcon.svelte'; import { bookmarkHostname, type Bookmark } from '$lib/dashboard/types'; - let { bookmarks }: { bookmarks: Bookmark[] } = $props(); + let { + bookmarks + }: { + /** Null when loading them failed: the section says so instead of listing them. */ + bookmarks: Bookmark[] | null; + } = $props(); let addModalOpen = $state(false); let editingBookmark: Bookmark | null = $state(null); @@ -41,7 +46,9 @@ } - +{#if bookmarks} + +{/if}
@@ -53,21 +60,27 @@ > {m.bookmark_section_title()} - {#if bookmarks.length > 0} + {#if bookmarks?.length} {m.bookmark_count_links({ count: bookmarks.length })} {/if}
-

{m.dashboard_subtitle()}

+ {#if bookmarks} +

{m.dashboard_subtitle()}

+ {/if} - + {#if bookmarks} + + {/if} - {#if bookmarks.length > 0} + {#if !bookmarks} +
{m.bookmark_load_error()}
+ {:else if bookmarks.length > 0}
{#each bookmarks as bookmark (bookmark.id)} {@const product = getProduct(bookmark.productId)} diff --git a/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts b/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts index cb8b9808..dffbe344 100644 --- a/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts +++ b/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts @@ -39,6 +39,21 @@ describe('DashboardBookmarks', () => { await expect.element(page.getByRole('button', { name: /add bookmark/i })).toBeInTheDocument(); }); + it('shows a message instead of the list, without adding, when loading failed', async () => { + render(DashboardBookmarks, { bookmarks: null }); + + await expect + .element(page.getByRole('heading', { level: 3, name: 'Bookmarks' })) + .toBeInTheDocument(); + await expect + .element(page.getByText('Bookmarks could not be loaded. Please reload the page later.')) + .toBeVisible(); + await expect + .element(page.getByRole('button', { name: /add bookmark/i })) + .not.toBeInTheDocument(); + await expect.element(page.getByText(/no bookmarks yet/i)).not.toBeInTheDocument(); + }); + it('shows an empty state when there are no bookmarks', async () => { render(DashboardBookmarks, { bookmarks: [] }); diff --git a/src/routes/(app)/+layout.server.ts b/src/routes/(app)/+layout.server.ts index b025a0d4..2c13f544 100644 --- a/src/routes/(app)/+layout.server.ts +++ b/src/routes/(app)/+layout.server.ts @@ -6,23 +6,25 @@ import { BOOKMARKS_DEPENDENCY, type Bookmark } from '$lib/dashboard/types'; export const load: LayoutServerLoad = async ({ locals, depends }) => { depends(BOOKMARKS_DEPENDENCY); - return { user: locals.user, storageBrowserEnabled, bookmarks: await loadBookmarks(locals) }; + return { user: locals.user, storageBrowserEnabled, ...(await loadBookmarks(locals)) }; }; /** - * The signed-in user's bookmarks, or null when bookmarks are unavailable: - * without OIDC there is no user to store them for, and a database failure - * must not take down every page of the app. + * The signed-in user's bookmarks, or null when they are unavailable: without + * OIDC there is no user to store them for, and a database failure must not + * take down every page of the app; `bookmarksFailed` tells the two apart. */ -async function loadBookmarks(locals: App.Locals): Promise { - if (!locals.user) return null; +async function loadBookmarks( + locals: App.Locals +): Promise<{ bookmarks: Bookmark[] | null; bookmarksFailed: boolean }> { + if (!locals.user) return { bookmarks: null, bookmarksFailed: false }; try { const bookmarks = await listBookmarks(locals.user.id); dashboardBookmarksTotal.inc({ operation: 'load', outcome: 'success' }); - return bookmarks; + return { bookmarks, bookmarksFailed: false }; } catch (err) { dashboardBookmarksTotal.inc({ operation: 'load', outcome: 'error' }); locals.logger.error({ err, user_id: locals.user.id }, 'Failed to load bookmarks'); - return null; + return { bookmarks: null, bookmarksFailed: true }; } } diff --git a/src/routes/(app)/+page.svelte b/src/routes/(app)/+page.svelte index 936cd97f..99b18442 100644 --- a/src/routes/(app)/+page.svelte +++ b/src/routes/(app)/+page.svelte @@ -8,9 +8,9 @@

{m.dashboard_welcome()}

- - {#if data.bookmarks} + + {#if data.bookmarks || data.bookmarksFailed} {/if}
diff --git a/src/routes/(app)/layout.server.test.ts b/src/routes/(app)/layout.server.test.ts index 7ade1ebc..885c95ee 100644 --- a/src/routes/(app)/layout.server.test.ts +++ b/src/routes/(app)/layout.server.test.ts @@ -33,7 +33,7 @@ describe('(app) layout load', () => { const data = await load(event); - expect(data).toMatchObject({ bookmarks: [BOOKMARK] }); + expect(data).toMatchObject({ bookmarks: [BOOKMARK], bookmarksFailed: false }); expect(listBookmarks).toHaveBeenCalledWith('user-1'); expect(event.depends).toHaveBeenCalledWith('app:bookmarks'); }); @@ -41,17 +41,17 @@ describe('(app) layout load', () => { it('has no bookmarks without a signed-in user', async () => { const data = await load(mockEvent(undefined)); - expect(data).toMatchObject({ bookmarks: null }); + expect(data).toMatchObject({ bookmarks: null, bookmarksFailed: false }); expect(listBookmarks).not.toHaveBeenCalled(); }); - it('logs and hides bookmarks instead of failing the page when the database fails', async () => { + it('logs and reports the failure instead of failing the page when the database fails', async () => { listBookmarks.mockRejectedValue(new Error('connection refused')); const event = mockEvent({ id: 'user-1' }); const data = await load(event); - expect(data).toMatchObject({ bookmarks: null }); + expect(data).toMatchObject({ bookmarks: null, bookmarksFailed: true }); expect(event.locals.logger.error).toHaveBeenCalled(); }); }); diff --git a/src/routes/(app)/page.svelte.spec.ts b/src/routes/(app)/page.svelte.spec.ts index 07f4cb5d..2f1ce1a8 100644 --- a/src/routes/(app)/page.svelte.spec.ts +++ b/src/routes/(app)/page.svelte.spec.ts @@ -4,9 +4,9 @@ import { render } from 'vitest-browser-svelte'; import type { Bookmark } from '$lib/dashboard/types'; import Page from './+page.svelte'; -const renderPage = (bookmarks: Bookmark[] | null = []) => +const renderPage = (bookmarks: Bookmark[] | null = [], bookmarksFailed = false) => render(Page, { - data: { user: undefined, storageBrowserEnabled: false, bookmarks } + data: { user: undefined, storageBrowserEnabled: false, bookmarks, bookmarksFailed } } as unknown as Parameters>[1]); describe('/(app)/+page.svelte', () => { @@ -22,7 +22,18 @@ describe('/(app)/+page.svelte', () => { .toBeInTheDocument(); }); - it('hides everything bookmark-related when bookmarks are unavailable', async () => { + it('says so when loading the bookmarks failed', async () => { + renderPage(null, true); + + await expect + .element(page.getByText('Bookmarks could not be loaded. Please reload the page later.')) + .toBeVisible(); + await expect + .element(page.getByRole('button', { name: /add bookmark/i })) + .not.toBeInTheDocument(); + }); + + it('hides everything bookmark-related without a signed-in user', async () => { renderPage(null); await expect.element(page.getByRole('heading', { level: 2 })).toBeInTheDocument(); From 9828eff652e9a463d81c25dc4b44946cb6f481d7 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Tue, 27 Oct 2026 16:32:01 +0100 Subject: [PATCH 5/8] fix: keep the bookmark list in sync after failed requests (#277) A failed request now reloads the list, e.g. after a change in another tab. A reload that fails after a saved change is retried once, then a toast offers a page reload instead of leaving the list stale. --- messages/de.json | 2 ++ messages/en.json | 2 ++ src/lib/dashboard/bookmarks-api.test.ts | 40 +++++++++++++++++++----- src/lib/dashboard/bookmarks-api.ts | 41 ++++++++++++++++++++----- 4 files changed, 70 insertions(+), 15 deletions(-) diff --git a/messages/de.json b/messages/de.json index d64654e3..9e0d49b6 100644 --- a/messages/de.json +++ b/messages/de.json @@ -502,6 +502,8 @@ "bookmark_delete_error": "Das Lesezeichen konnte nicht gelöscht werden. Bitte versuchen Sie es erneut.", "bookmark_pin_error": "Die Anheftung von „{name}“ konnte nicht geändert werden. Bitte versuchen Sie es erneut.", "bookmark_load_error": "Die Lesezeichen konnten nicht geladen werden. Bitte laden Sie die Seite später neu.", + "bookmark_refresh_error": "Die Änderung wurde gespeichert, aber die Lesezeichenliste konnte nicht aktualisiert werden.", + "bookmark_reload": "Neu laden", "bookmark_product_custom": "Eigener Link", "bookmark_default_name_trino": "SQL-Editor", "bookmark_default_name_superset": "Dashboards", diff --git a/messages/en.json b/messages/en.json index b8f70309..970e9ea3 100644 --- a/messages/en.json +++ b/messages/en.json @@ -502,6 +502,8 @@ "bookmark_delete_error": "The bookmark could not be deleted. Please try again.", "bookmark_pin_error": "Could not update the pin for \"{name}\". Please try again.", "bookmark_load_error": "Bookmarks could not be loaded. Please reload the page later.", + "bookmark_refresh_error": "The change was saved, but the bookmark list could not be updated.", + "bookmark_reload": "Reload", "bookmark_product_custom": "Custom Link", "bookmark_default_name_trino": "SQL Editor", "bookmark_default_name_superset": "Dashboards", diff --git a/src/lib/dashboard/bookmarks-api.test.ts b/src/lib/dashboard/bookmarks-api.test.ts index 018b5a60..6c92d762 100644 --- a/src/lib/dashboard/bookmarks-api.test.ts +++ b/src/lib/dashboard/bookmarks-api.test.ts @@ -1,10 +1,16 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -const { invalidate } = vi.hoisted(() => ({ invalidate: vi.fn() })); +const { invalidate, addToast } = vi.hoisted(() => ({ invalidate: vi.fn(), addToast: vi.fn() })); vi.mock('$app/navigation', () => ({ invalidate })); +vi.mock('$lib/stores/toast.svelte.js', () => ({ addToast })); -import { addBookmark, updateBookmark, removeBookmark } from './bookmarks-api.js'; +import { + addBookmark, + BookmarkRequestError, + updateBookmark, + removeBookmark +} from './bookmarks-api.js'; const fetchMock = vi.fn(); vi.stubGlobal('fetch', fetchMock); @@ -50,18 +56,36 @@ describe('bookmarks-api', () => { ]); expect(invalidate).toHaveBeenCalledTimes(3); expect(invalidate).toHaveBeenCalledWith('app:bookmarks'); + expect(addToast).not.toHaveBeenCalled(); }); - it('rejects without reloading when the server refuses the change', async () => { - fetchMock.mockResolvedValue(new Response(null, { status: 500 })); + it('reloads the list and rejects with the status when the server refuses the change', async () => { + fetchMock.mockResolvedValue(new Response(null, { status: 404 })); - await expect(removeBookmark('b1')).rejects.toThrow('500'); - expect(invalidate).not.toHaveBeenCalled(); + const result = removeBookmark('b1'); + + await expect(result).rejects.toBeInstanceOf(BookmarkRequestError); + await expect(result).rejects.toMatchObject({ status: 404 }); + expect(invalidate).toHaveBeenCalledWith('app:bookmarks'); }); - it('resolves when the change is saved but reloading the list fails', async () => { - invalidate.mockRejectedValue(new Error('session expired')); + it('retries a failed reload once', async () => { + invalidate.mockRejectedValueOnce(new Error('network')).mockResolvedValueOnce(undefined); + + await addBookmark(FIELDS); + + expect(invalidate).toHaveBeenCalledTimes(2); + expect(addToast).not.toHaveBeenCalled(); + }); + + it('resolves and offers a page reload when the change is saved but the list cannot be reloaded', async () => { + invalidate.mockRejectedValue(new Error('network')); await expect(addBookmark(FIELDS)).resolves.toBeUndefined(); + + expect(invalidate).toHaveBeenCalledTimes(2); + expect(addToast).toHaveBeenCalledWith('warning', expect.any(String), 0, [ + { label: 'Reload', onClick: expect.any(Function) } + ]); }); }); diff --git a/src/lib/dashboard/bookmarks-api.ts b/src/lib/dashboard/bookmarks-api.ts index 4151e733..114feb8c 100644 --- a/src/lib/dashboard/bookmarks-api.ts +++ b/src/lib/dashboard/bookmarks-api.ts @@ -1,10 +1,32 @@ import { invalidate } from '$app/navigation'; +import * as m from '$lib/paraglide/messages.js'; +import { addToast } from '$lib/stores/toast.svelte.js'; import { BOOKMARKS_DEPENDENCY, type Bookmark, type BookmarkUpdate } from './types'; // The bookmarks live in the `(app)` layout data rather than a module-level -// store, which the server would share between users during SSR. Every change +// store, which the server would share between users during SSR. Every request // reloads them from there. +/** A bookmark request the server answered with an error status. */ +export class BookmarkRequestError extends Error { + constructor(readonly status: number) { + super(`Bookmark request failed with status ${status}`); + } +} + +/** Reload the layout's bookmarks, with one retry; false if both attempts failed. */ +async function refresh(): Promise { + for (let attempt = 0; attempt < 2; attempt++) { + try { + await invalidate(BOOKMARKS_DEPENDENCY); + return true; + } catch { + // Retry once, then give up. + } + } + return false; +} + async function send(path: string, method: string, body?: unknown): Promise { const response = await fetch(path, { method, @@ -13,13 +35,18 @@ async function send(path: string, method: string, body?: unknown): Promise : { headers: { 'content-type': 'application/json' }, body: JSON.stringify(body) }) }); if (!response.ok) { - throw new Error(`Bookmark request failed with status ${response.status}`); + // The list may be outdated, e.g. after a change in another tab, which is + // often why the request failed. + await refresh(); + throw new BookmarkRequestError(response.status); } - try { - await invalidate(BOOKMARKS_DEPENDENCY); - } catch { - // The change is saved; reporting it as failed would invite a duplicate. - // The list catches up on the next navigation. + // The change is saved, so a failed reload must not be reported as a failed + // save, which would invite a duplicate. The layout load does not rerun on + // navigation, so only a full reload brings the list up to date. + if (!(await refresh())) { + addToast('warning', m.bookmark_refresh_error(), 0, [ + { label: m.bookmark_reload(), onClick: () => location.reload() } + ]); } } From 68afd15507f573b923495053365b315fecec8ba6 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Tue, 27 Oct 2026 16:32:11 +0100 Subject: [PATCH 6/8] feat: name the bookmark limit and announce saving in the dialog (#277) A 409 shows the limit instead of a generic error, and a status region announces the save to screen readers while the spinner is shown. --- messages/de.json | 2 ++ messages/en.json | 2 ++ .../dashboard/AddBookmarkModal.svelte | 22 +++++++++++++++---- .../dashboard/AddBookmarkModal.svelte.spec.ts | 21 +++++++++++++++++- .../DashboardBookmarks.svelte.spec.ts | 3 ++- 5 files changed, 44 insertions(+), 6 deletions(-) diff --git a/messages/de.json b/messages/de.json index 9e0d49b6..ca5ce12d 100644 --- a/messages/de.json +++ b/messages/de.json @@ -504,6 +504,8 @@ "bookmark_load_error": "Die Lesezeichen konnten nicht geladen werden. Bitte laden Sie die Seite später neu.", "bookmark_refresh_error": "Die Änderung wurde gespeichert, aber die Lesezeichenliste konnte nicht aktualisiert werden.", "bookmark_reload": "Neu laden", + "bookmark_limit_error": "Sie können bis zu {limit} Lesezeichen anlegen. Löschen Sie eines, um ein weiteres hinzuzufügen.", + "bookmark_saving": "Wird gespeichert…", "bookmark_product_custom": "Eigener Link", "bookmark_default_name_trino": "SQL-Editor", "bookmark_default_name_superset": "Dashboards", diff --git a/messages/en.json b/messages/en.json index 970e9ea3..47238975 100644 --- a/messages/en.json +++ b/messages/en.json @@ -504,6 +504,8 @@ "bookmark_load_error": "Bookmarks could not be loaded. Please reload the page later.", "bookmark_refresh_error": "The change was saved, but the bookmark list could not be updated.", "bookmark_reload": "Reload", + "bookmark_limit_error": "You can have up to {limit} bookmarks. Delete one to add another.", + "bookmark_saving": "Saving…", "bookmark_product_custom": "Custom Link", "bookmark_default_name_trino": "SQL Editor", "bookmark_default_name_superset": "Dashboards", diff --git a/src/lib/components/dashboard/AddBookmarkModal.svelte b/src/lib/components/dashboard/AddBookmarkModal.svelte index 591b5f90..64b13b91 100644 --- a/src/lib/components/dashboard/AddBookmarkModal.svelte +++ b/src/lib/components/dashboard/AddBookmarkModal.svelte @@ -3,8 +3,14 @@ import Modal from '$lib/components/Modal.svelte'; import { getProduct, PRODUCTS, type Product } from '$lib/dashboard/products'; import ProductIcon from './ProductIcon.svelte'; - import { addBookmark, removeBookmark, updateBookmark } from '$lib/dashboard/bookmarks-api.js'; import { + addBookmark, + BookmarkRequestError, + removeBookmark, + updateBookmark + } from '$lib/dashboard/bookmarks-api.js'; + import { + BOOKMARK_LIMIT, BOOKMARK_MAX_LENGTH, bookmarkCreateSchema, bookmarkHostname, @@ -82,15 +88,21 @@ if (url.trim()) urlError = !isValidUrl(url); } - /** Runs a bookmark request; closes the dialog on success, otherwise shows `errorMessage`. */ + /** + * Runs a bookmark request; closes the dialog on success, otherwise shows + * `errorMessage`, or the limit when the server rejects another bookmark. + */ async function save(request: () => Promise, errorMessage: string) { saving = true; requestError = null; try { await request(); open = false; - } catch { - requestError = errorMessage; + } catch (err) { + requestError = + err instanceof BookmarkRequestError && err.status === 409 + ? m.bookmark_limit_error({ limit: BOOKMARK_LIMIT }) + : errorMessage; } finally { saving = false; } @@ -298,6 +310,8 @@ {#if requestError} {/if} + +

{saving ? m.bookmark_saving() : ''}

{#if isEditing} diff --git a/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts b/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts index cba8b91e..6f091e0b 100644 --- a/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts +++ b/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts @@ -14,7 +14,12 @@ const { addBookmark, updateBookmark, removeBookmark } = vi.hoisted(() => ({ vi.mock('$lib/dashboard/bookmarks-api.js', () => ({ addBookmark, updateBookmark, - removeBookmark + removeBookmark, + BookmarkRequestError: class extends Error { + constructor(readonly status: number) { + super(`status ${status}`); + } + } })); const renderModal = (props: Record = {}) => @@ -292,6 +297,19 @@ describe('AddBookmarkModal', () => { await expect.element(page.getByLabelText('Name')).toHaveValue('Unsaved Dashboard'); }); + it('names the limit when the server rejects another bookmark', async () => { + const { BookmarkRequestError } = await import('$lib/dashboard/bookmarks-api.js'); + addBookmark.mockRejectedValue(new BookmarkRequestError(409)); + renderModal(); + + await page.getByLabelText('URL').fill('https://superset.example.com'); + await page.getByRole('button', { name: 'Add bookmark' }).click(); + + await expect + .element(page.getByRole('alert')) + .toHaveTextContent('You can have up to 25 bookmarks. Delete one to add another.'); + }); + it('returns to the edit dialog with an error when deleting fails', async () => { removeBookmark.mockRejectedValue(new Error('500')); renderModal({ bookmark: savedBookmark }); @@ -322,5 +340,6 @@ describe('AddBookmarkModal', () => { expect(addBookmark).toHaveBeenCalledTimes(1); await expect.element(page.getByRole('dialog', { name: 'Add bookmark' })).toBeVisible(); + await expect.element(page.getByRole('status')).toHaveTextContent('Saving…'); }); }); diff --git a/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts b/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts index dffbe344..fb4cf75e 100644 --- a/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts +++ b/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts @@ -12,7 +12,8 @@ const { updateBookmark, addToast } = vi.hoisted(() => ({ vi.mock('$lib/dashboard/bookmarks-api.js', () => ({ addBookmark: vi.fn(), updateBookmark, - removeBookmark: vi.fn() + removeBookmark: vi.fn(), + BookmarkRequestError: class extends Error {} })); vi.mock('$lib/stores/toast.svelte.js', () => ({ addToast })); From 91476c143a6a28b5e791d0682d96cbcbf9dc9a44 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Wed, 28 Oct 2026 16:49:11 +0100 Subject: [PATCH 7/8] fix: drop the bookmark reload retry that never ran (#277) SvelteKit's invalidate() does not reject when the reload fails: it shows its error page and resolves. The retry and the "Reload" toast after a saved change therefore never ran, and their unit tests faked a rejection that does not happen. Verified with Playwright by failing __data.json after a successful save: one request, no toast, bare 500 page. When the server refuses a change, start the reload without awaiting it, so the dialog shows the error without waiting for the reload. --- messages/de.json | 2 -- messages/en.json | 2 -- src/lib/dashboard/bookmarks-api.test.ts | 24 +---------------- src/lib/dashboard/bookmarks-api.ts | 34 +++++-------------------- 4 files changed, 8 insertions(+), 54 deletions(-) diff --git a/messages/de.json b/messages/de.json index 8ddc4d5b..b1f271a5 100644 --- a/messages/de.json +++ b/messages/de.json @@ -512,8 +512,6 @@ "bookmark_delete_error": "Das Lesezeichen konnte nicht gelöscht werden. Bitte versuchen Sie es erneut.", "bookmark_pin_error": "Die Anheftung von „{name}“ konnte nicht geändert werden. Bitte versuchen Sie es erneut.", "bookmark_load_error": "Die Lesezeichen konnten nicht geladen werden. Bitte laden Sie die Seite später neu.", - "bookmark_refresh_error": "Die Änderung wurde gespeichert, aber die Lesezeichenliste konnte nicht aktualisiert werden.", - "bookmark_reload": "Neu laden", "bookmark_limit_error": "Sie können bis zu {limit} Lesezeichen anlegen. Löschen Sie eines, um ein weiteres hinzuzufügen.", "bookmark_saving": "Wird gespeichert…", "bookmark_product_custom": "Eigener Link", diff --git a/messages/en.json b/messages/en.json index a71b1099..babbc090 100644 --- a/messages/en.json +++ b/messages/en.json @@ -512,8 +512,6 @@ "bookmark_delete_error": "The bookmark could not be deleted. Please try again.", "bookmark_pin_error": "Could not update the pin for \"{name}\". Please try again.", "bookmark_load_error": "Bookmarks could not be loaded. Please reload the page later.", - "bookmark_refresh_error": "The change was saved, but the bookmark list could not be updated.", - "bookmark_reload": "Reload", "bookmark_limit_error": "You can have up to {limit} bookmarks. Delete one to add another.", "bookmark_saving": "Saving…", "bookmark_product_custom": "Custom Link", diff --git a/src/lib/dashboard/bookmarks-api.test.ts b/src/lib/dashboard/bookmarks-api.test.ts index 6c92d762..b975aed6 100644 --- a/src/lib/dashboard/bookmarks-api.test.ts +++ b/src/lib/dashboard/bookmarks-api.test.ts @@ -1,9 +1,8 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -const { invalidate, addToast } = vi.hoisted(() => ({ invalidate: vi.fn(), addToast: vi.fn() })); +const { invalidate } = vi.hoisted(() => ({ invalidate: vi.fn() })); vi.mock('$app/navigation', () => ({ invalidate })); -vi.mock('$lib/stores/toast.svelte.js', () => ({ addToast })); import { addBookmark, @@ -56,7 +55,6 @@ describe('bookmarks-api', () => { ]); expect(invalidate).toHaveBeenCalledTimes(3); expect(invalidate).toHaveBeenCalledWith('app:bookmarks'); - expect(addToast).not.toHaveBeenCalled(); }); it('reloads the list and rejects with the status when the server refuses the change', async () => { @@ -68,24 +66,4 @@ describe('bookmarks-api', () => { await expect(result).rejects.toMatchObject({ status: 404 }); expect(invalidate).toHaveBeenCalledWith('app:bookmarks'); }); - - it('retries a failed reload once', async () => { - invalidate.mockRejectedValueOnce(new Error('network')).mockResolvedValueOnce(undefined); - - await addBookmark(FIELDS); - - expect(invalidate).toHaveBeenCalledTimes(2); - expect(addToast).not.toHaveBeenCalled(); - }); - - it('resolves and offers a page reload when the change is saved but the list cannot be reloaded', async () => { - invalidate.mockRejectedValue(new Error('network')); - - await expect(addBookmark(FIELDS)).resolves.toBeUndefined(); - - expect(invalidate).toHaveBeenCalledTimes(2); - expect(addToast).toHaveBeenCalledWith('warning', expect.any(String), 0, [ - { label: 'Reload', onClick: expect.any(Function) } - ]); - }); }); diff --git a/src/lib/dashboard/bookmarks-api.ts b/src/lib/dashboard/bookmarks-api.ts index 114feb8c..735d46f8 100644 --- a/src/lib/dashboard/bookmarks-api.ts +++ b/src/lib/dashboard/bookmarks-api.ts @@ -1,11 +1,10 @@ import { invalidate } from '$app/navigation'; -import * as m from '$lib/paraglide/messages.js'; -import { addToast } from '$lib/stores/toast.svelte.js'; import { BOOKMARKS_DEPENDENCY, type Bookmark, type BookmarkUpdate } from './types'; // The bookmarks live in the `(app)` layout data rather than a module-level // store, which the server would share between users during SSR. Every request -// reloads them from there. +// reloads them from there. `invalidate` does not reject when that reload fails: +// SvelteKit shows its error page instead, so there is no failure to handle here. /** A bookmark request the server answered with an error status. */ export class BookmarkRequestError extends Error { @@ -14,19 +13,6 @@ export class BookmarkRequestError extends Error { } } -/** Reload the layout's bookmarks, with one retry; false if both attempts failed. */ -async function refresh(): Promise { - for (let attempt = 0; attempt < 2; attempt++) { - try { - await invalidate(BOOKMARKS_DEPENDENCY); - return true; - } catch { - // Retry once, then give up. - } - } - return false; -} - async function send(path: string, method: string, body?: unknown): Promise { const response = await fetch(path, { method, @@ -35,19 +21,13 @@ async function send(path: string, method: string, body?: unknown): Promise : { headers: { 'content-type': 'application/json' }, body: JSON.stringify(body) }) }); if (!response.ok) { - // The list may be outdated, e.g. after a change in another tab, which is - // often why the request failed. - await refresh(); + // Reload on failure too: the list may be outdated, e.g. after a change in + // another tab, which is often why the request failed. Not awaited, so the + // caller can show the error without waiting for the reload. + void invalidate(BOOKMARKS_DEPENDENCY); throw new BookmarkRequestError(response.status); } - // The change is saved, so a failed reload must not be reported as a failed - // save, which would invite a duplicate. The layout load does not rerun on - // navigation, so only a full reload brings the list up to date. - if (!(await refresh())) { - addToast('warning', m.bookmark_refresh_error(), 0, [ - { label: m.bookmark_reload(), onClick: () => location.reload() } - ]); - } + await invalidate(BOOKMARKS_DEPENDENCY); } export function addBookmark(bookmark: Omit): Promise { From f1340e41e0c53f2b8dfda66d436b075d0a62335d Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Wed, 28 Oct 2026 16:49:14 +0100 Subject: [PATCH 8/8] test: check that the sidebar follows pin changes without a reload (#277) All sidebar tests seeded bookmarks and then loaded the page, so the invalidate path from a dashboard change to the layout's bookmarks was untested end to end. --- e2e/sidebar-bookmarks.spec.ts | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/e2e/sidebar-bookmarks.spec.ts b/e2e/sidebar-bookmarks.spec.ts index 016cb855..e361554e 100644 --- a/e2e/sidebar-bookmarks.spec.ts +++ b/e2e/sidebar-bookmarks.spec.ts @@ -65,6 +65,28 @@ test.describe('Sidebar bookmarks', () => { await expect(trinoLink.locator('..').getByRole('link')).toHaveCount(1); }); + test('follows pin changes made on the dashboard without a page reload', async ({ page }) => { + await page.goto('/'); + await waitForHydration(page); + // Lost on a full page load, so it proves the sidebar updated in place. + await page.evaluate(() => ((window as unknown as { noReload: boolean }).noReload = true)); + + // With Tools collapsed, only pinned bookmarks remain visible in the sidebar. + await page.getByRole('button', { name: 'Tools', exact: true }).click(); + const dashboardsLink = sidebarNav(page).getByRole('link', { name: 'Dashboards' }); + await expect(dashboardsLink).not.toBeVisible(); + + await page.getByRole('button', { name: 'Pin bookmark Dashboards' }).click(); + await expect(dashboardsLink).toBeVisible(); + + await page.getByRole('button', { name: 'Unpin bookmark Dashboards' }).click(); + await expect(dashboardsLink).not.toBeVisible(); + + expect(await page.evaluate(() => (window as unknown as { noReload?: boolean }).noReload)).toBe( + true + ); + }); + test('tools section is collapsible', async ({ page }) => { await page.goto('/'); await waitForHydration(page);