diff --git a/e2e/bookmarks.spec.ts b/e2e/bookmarks.spec.ts index 111ffaa2..61260a00 100644 --- a/e2e/bookmarks.spec.ts +++ b/e2e/bookmarks.spec.ts @@ -1,100 +1,54 @@ 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' }); const addButton = 'Add bookmark'; test.beforeEach(async ({ page }) => { - await page.addInitScript(() => { - localStorage.removeItem('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.afterEach(async ({ page }) => { + await resetBookmarks(page); }); - 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 }) => { @@ -118,25 +72,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); @@ -150,325 +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 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 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('bookmark persists in localStorage', async ({ page }) => { - 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(); - - 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'); - }); - - 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(); - - 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); - }); - - 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 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(); - - 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 }) => { - 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(); - - 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 }) => { - 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/e2e/sidebar-bookmarks.spec.ts b/e2e/sidebar-bookmarks.spec.ts index 4977f604..e361554e 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,28 +65,26 @@ 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() - } - ]) - ); - }); - + 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)); - await expect(page.getByRole('link', { name: 'Unsafe link' })).not.toBeVisible(); + // 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 }) => { 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 3c0979e1..b1f271a5 100644 --- a/messages/de.json +++ b/messages/de.json @@ -508,6 +508,12 @@ "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_load_error": "Die Lesezeichen konnten nicht geladen werden. Bitte laden Sie die Seite später neu.", + "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 3392087d..babbc090 100644 --- a/messages/en.json +++ b/messages/en.json @@ -508,6 +508,12 @@ "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_load_error": "Bookmarks could not be loaded. Please reload the page later.", + "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 469da0da..b302a5b9 100644 --- a/src/lib/components/dashboard/AddBookmarkModal.svelte +++ b/src/lib/components/dashboard/AddBookmarkModal.svelte @@ -3,10 +3,16 @@ 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, + BookmarkRequestError, + removeBookmark, + updateBookmark + } from '$lib/dashboard/bookmarks-api.js'; + import { + BOOKMARK_LIMIT, BOOKMARK_MAX_LENGTH, - bookmarkFormSchema, + bookmarkCreateSchema, bookmarkHostname, type Bookmark } from '$lib/dashboard/types'; @@ -31,6 +37,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 +53,7 @@ pinned = target?.pinned ?? false; nameError = false; urlError = false; + requestError = null; } // Only `open` is tracked; the bookmark is read once per open. @@ -72,15 +81,36 @@ } 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`, 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 (err) { + requestError = + err instanceof BookmarkRequestError && err.status === 409 + ? m.bookmark_limit_error({ limit: BOOKMARK_LIMIT }) + : errorMessage; + } finally { + saving = false; + } + } + + async function handleSubmit(event: SubmitEvent) { event.preventDefault(); + if (saving) return; const values = { productId: selectedProduct.id, name: name.trim(), @@ -88,7 +118,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 +126,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 +148,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()); } - + + + {#if requestError} + + {/if} + +

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

+ diff --git a/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts b/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts index 389f6946..6f091e0b 100644 --- a/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts +++ b/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts @@ -11,10 +11,15 @@ 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 + removeBookmark, + BookmarkRequestError: class extends Error { + constructor(readonly status: number) { + super(`status ${status}`); + } + } })); const renderModal = (props: Record = {}) => @@ -39,6 +44,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 () => { @@ -98,6 +106,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(); @@ -113,6 +169,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(); @@ -175,6 +242,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 }); @@ -196,7 +278,68 @@ 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('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 }); + + 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(); + await expect.element(page.getByRole('status')).toHaveTextContent('Saving…'); + }); }); diff --git a/src/lib/components/dashboard/DashboardBookmarks.svelte b/src/lib/components/dashboard/DashboardBookmarks.svelte new file mode 100644 index 00000000..eb7288ec --- /dev/null +++ b/src/lib/components/dashboard/DashboardBookmarks.svelte @@ -0,0 +1,159 @@ + + +{#if bookmarks} + +{/if} + +
+
+
+
+

+ {m.bookmark_section_title()} +

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

{m.dashboard_subtitle()}

+ {/if} +
+ {#if bookmarks} + + {/if} +
+ + {#if !bookmarks} +
{m.bookmark_load_error()}
+ {:else if bookmarks.length > 0} +
+ {#each bookmarks as bookmark (bookmark.id)} + {@const product = getProduct(bookmark.productId)} +
+ + +
+ +
+ + {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..fb4cf75e --- /dev/null +++ b/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts @@ -0,0 +1,163 @@ +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(), + BookmarkRequestError: class extends Error {} +})); +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 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: [] }); + + await expect.element(page.getByText(/no bookmarks yet/i)).toBeInTheDocument(); + }); + + 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', 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: [] }); + + 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('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] }); + + 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/lib/components/layout/sidebar/Sidebar.svelte b/src/lib/components/layout/sidebar/Sidebar.svelte index ef737206..dfcf3c2a 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..b975aed6 --- /dev/null +++ b/src/lib/dashboard/bookmarks-api.test.ts @@ -0,0 +1,69 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const { invalidate } = vi.hoisted(() => ({ invalidate: vi.fn() })); + +vi.mock('$app/navigation', () => ({ invalidate })); + +import { + addBookmark, + BookmarkRequestError, + 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('reloads the list and rejects with the status when the server refuses the change', async () => { + fetchMock.mockResolvedValue(new Response(null, { status: 404 })); + + const result = removeBookmark('b1'); + + await expect(result).rejects.toBeInstanceOf(BookmarkRequestError); + await expect(result).rejects.toMatchObject({ status: 404 }); + expect(invalidate).toHaveBeenCalledWith('app:bookmarks'); + }); +}); diff --git a/src/lib/dashboard/bookmarks-api.ts b/src/lib/dashboard/bookmarks-api.ts new file mode 100644 index 00000000..735d46f8 --- /dev/null +++ b/src/lib/dashboard/bookmarks-api.ts @@ -0,0 +1,43 @@ +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 request +// 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 { + constructor(readonly status: number) { + super(`Bookmark request failed with status ${status}`); + } +} + +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) { + // 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); + } + await invalidate(BOOKMARKS_DEPENDENCY); +} + +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..2c13f544 100644 --- a/src/routes/(app)/+layout.server.ts +++ b/src/routes/(app)/+layout.server.ts @@ -1,6 +1,30 @@ 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, ...(await loadBookmarks(locals)) }; }; + +/** + * 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<{ 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, bookmarksFailed: false }; + } catch (err) { + dashboardBookmarksTotal.inc({ operation: 'load', outcome: 'error' }); + locals.logger.error({ err, user_id: locals.user.id }, 'Failed to load bookmarks'); + return { bookmarks: null, bookmarksFailed: true }; + } +} 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 68985c20..99b18442 100644 --- a/src/routes/(app)/+page.svelte +++ b/src/routes/(app)/+page.svelte @@ -1,132 +1,16 @@ - -
-
-
-

{m.dashboard_welcome()}

-

{m.dashboard_subtitle()}

-
- -
- -
-
-

- {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)} -
- - -
- -
- - {product.name} - - {#if bookmark.environment} - - {bookmark.environment} - - {/if} -
-
- - {bookmarkHostname(bookmark.url)} - -
-
- - -
- {/each} -
- {:else} -
-

{m.dashboard_bookmarks_empty()}

-
- {/if} -
+ + {#if data.bookmarks || data.bookmarksFailed} + + {/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..885c95ee --- /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], bookmarksFailed: false }); + 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, bookmarksFailed: false }); + expect(listBookmarks).not.toHaveBeenCalled(); + }); + + 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, 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 88ad9bbc..2f1ce1a8 100644 --- a/src/routes/(app)/page.svelte.spec.ts +++ b/src/routes/(app)/page.svelte.spec.ts @@ -1,9 +1,13 @@ import { page } from 'vitest/browser'; 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 renderPage = () => render(Page); +const renderPage = (bookmarks: Bookmark[] | null = [], bookmarksFailed = false) => + render(Page, { + data: { user: undefined, storageBrowserEnabled: false, bookmarks, bookmarksFailed } + } as unknown as Parameters>[1]); describe('/(app)/+page.svelte', () => { it('should render the platform heading', async () => { @@ -11,25 +15,32 @@ describe('/(app)/+page.svelte', () => { await expect.element(page.getByRole('heading', { level: 2 })).toBeInTheDocument(); }); - it('should render the dashboard subtitle', async () => { + it('should render the bookmarks section', async () => { renderPage(); - await expect.element(page.getByText(/bookmarks are added by hand/i)).toBeInTheDocument(); + await expect + .element(page.getByRole('heading', { level: 3, name: 'Bookmarks' })) + .toBeInTheDocument(); }); - it('should render the Add Bookmark button', async () => { - renderPage(); - await expect.element(page.getByRole('button', { name: /add bookmark/i })).toBeInTheDocument(); - }); + it('says so when loading the bookmarks failed', async () => { + renderPage(null, true); - it('should render the bookmarks section heading', async () => { - renderPage(); await expect - .element(page.getByRole('heading', { level: 3, name: 'Bookmarks' })) - .toBeInTheDocument(); + .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('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 without a signed-in user', 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(); }); });