From c543bc473e9b160a8ce57d65dee55dc84628315f Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Mon, 26 Oct 2026 12:04:59 +0100 Subject: [PATCH 1/5] fix: return {message} error bodies from all API routes (#359) The Trino query and storage connections endpoints answered errors with {error}, and the copy job endpoint with {status: 'not_found'}, while every other route returns SvelteKit's App.Error ({message}). Use error() there too and read the message on the client with a shared readErrorMessage helper, so the Trino submit shows the server's reason again. --- e2e/trino/query-api.spec.ts | 2 +- e2e/trino/trino.spec.ts | 2 +- src/lib/client/api-error.spec.ts | 18 +++++++++++ src/lib/client/api-error.ts | 20 ++++++++++++ src/lib/storage/api.spec.ts | 4 +-- src/lib/storage/storage-fetch.ts | 11 ++----- .../(app)/api/storage/connections/+server.ts | 8 ++--- .../api/storage/copy/job/[jobId]/+server.ts | 6 ++-- src/routes/(app)/api/trino/query/+server.ts | 12 +++---- .../(app)/api/trino/query/server.test.ts | 31 ++++++++----------- src/routes/(app)/trino/query-runner.svelte.ts | 4 +-- 11 files changed, 71 insertions(+), 47 deletions(-) create mode 100644 src/lib/client/api-error.spec.ts create mode 100644 src/lib/client/api-error.ts diff --git a/e2e/trino/query-api.spec.ts b/e2e/trino/query-api.spec.ts index 77878ad0..29babe63 100644 --- a/e2e/trino/query-api.spec.ts +++ b/e2e/trino/query-api.spec.ts @@ -81,7 +81,7 @@ test.describe('Trino API', () => { }); expect(res.status()).toBe(400); - expect(await res.json()).toEqual({ error: expect.any(String) }); + expect(await res.json()).toEqual({ message: expect.any(String) }); }); test('cancelling between statements stops the script', async ({ page }) => { diff --git a/e2e/trino/trino.spec.ts b/e2e/trino/trino.spec.ts index 2349e393..45f7ac08 100644 --- a/e2e/trino/trino.spec.ts +++ b/e2e/trino/trino.spec.ts @@ -125,7 +125,7 @@ test.describe('Trino query editor', () => { // Force the submit endpoint to fail with a server-provided reason. await page.route('**/api/trino/query', async (route, request) => { if (request.method() === 'POST') { - await route.fulfill({ status: 400, json: { error: 'Could not reach Trino' } }); + await route.fulfill({ status: 400, json: { message: 'Could not reach Trino' } }); } else { await route.continue(); } diff --git a/src/lib/client/api-error.spec.ts b/src/lib/client/api-error.spec.ts new file mode 100644 index 00000000..f3678294 --- /dev/null +++ b/src/lib/client/api-error.spec.ts @@ -0,0 +1,18 @@ +import { describe, expect, it } from 'vitest'; +import { readErrorMessage } from './api-error.js'; + +describe('readErrorMessage', () => { + it('returns the message of an App.Error body', async () => { + const res = Response.json({ message: 'Invalid JSON body' }, { status: 400 }); + + expect(await readErrorMessage(res)).toBe('Invalid JSON body'); + }); + + it.each([ + ['an HTML page', ''], + ['an empty body', ''], + ['JSON without a message', '{"error":"nope"}'] + ])('returns null for %s', async (_label, body) => { + expect(await readErrorMessage(new Response(body, { status: 502 }))).toBeNull(); + }); +}); diff --git a/src/lib/client/api-error.ts b/src/lib/client/api-error.ts new file mode 100644 index 00000000..1b5395d1 --- /dev/null +++ b/src/lib/client/api-error.ts @@ -0,0 +1,20 @@ +/** + * Read the message of an API error response. + * + * Every API route answers errors with an `App.Error` body (`{ message }`), + * produced by SvelteKit's `error()`, `handleError` or the auth guard. Returns + * `null` when the body is empty or not JSON, e.g. a proxy's HTML error page, + * so callers can fall back to a translated message. + */ +export async function readErrorMessage(res: Response): Promise { + const body: unknown = await res + .clone() + .json() + .catch(() => null); + return body !== null && + typeof body === 'object' && + 'message' in body && + typeof body.message === 'string' + ? body.message + : null; +} diff --git a/src/lib/storage/api.spec.ts b/src/lib/storage/api.spec.ts index d4b99387..fc4fe445 100644 --- a/src/lib/storage/api.spec.ts +++ b/src/lib/storage/api.spec.ts @@ -403,7 +403,7 @@ describe('createFetchStorageApi', () => { const api = createFetchStorageApi(() => 'conn-1'); vi.spyOn(globalThis, 'fetch') .mockResolvedValueOnce(jsonResponse({ status: 'done', items: [] })) - .mockResolvedValueOnce(jsonResponse({ status: 'not_found' }, { status: 404 })); + .mockResolvedValueOnce(jsonResponse({ message: 'Job not found' }, { status: 404 })); expect(await api.readTransferJob('job-1')).toEqual({ status: 'done', items: [] }); expect(await api.readTransferJob('job-2')).toEqual({ status: 'not_found' }); @@ -430,7 +430,7 @@ describe('createFetchStorageApi', () => { it('reports a missing job as not_found instead of throwing', async () => { const api = createFetchStorageApi(() => 'conn-1'); vi.spyOn(globalThis, 'fetch').mockResolvedValue( - jsonResponse({ status: 'not_found' }, { status: 404 }) + jsonResponse({ message: 'Job not found' }, { status: 404 }) ); await expect(api.pollJob('job-abc')).resolves.toEqual({ status: 'not_found' }); diff --git a/src/lib/storage/storage-fetch.ts b/src/lib/storage/storage-fetch.ts index 15b49754..195d7cd8 100644 --- a/src/lib/storage/storage-fetch.ts +++ b/src/lib/storage/storage-fetch.ts @@ -12,6 +12,7 @@ * const fetch_ = createStorageFetch(() => connectionStore.activeConnectionId); */ +import { readErrorMessage } from '$lib/client/api-error.js'; import { STORAGE_CONNECTION_ID_HEADER } from '$lib/storage/connection-id-header.js'; import { StorageError } from '$lib/storage/errors.js'; @@ -55,16 +56,8 @@ export function createStorageFetch( if (!response.ok) { const code = mapStatusToCode(response.status); - const body = await response - .clone() - .json() - .catch(() => null); const message = - body && typeof body === 'object' && 'message' in body && typeof body.message === 'string' - ? body.message - : body && typeof body === 'object' && 'error' in body && typeof body.error === 'string' - ? body.error - : `Request failed with status ${response.status}`; + (await readErrorMessage(response)) ?? `Request failed with status ${response.status}`; throw new StorageError(code, message); } diff --git a/src/routes/(app)/api/storage/connections/+server.ts b/src/routes/(app)/api/storage/connections/+server.ts index 8cacb054..dfa3449e 100644 --- a/src/routes/(app)/api/storage/connections/+server.ts +++ b/src/routes/(app)/api/storage/connections/+server.ts @@ -1,5 +1,5 @@ import { desc, eq, and } from 'drizzle-orm'; -import { json } from '@sveltejs/kit'; +import { error, json } from '@sveltejs/kit'; import { db } from '$lib/server/db.js'; import { userStorageConnections } from '$lib/server/schema.js'; import { STORAGE_CONNECTION_ID_HEADER } from '$lib/storage/connection-id-header.js'; @@ -72,12 +72,12 @@ export const PATCH: RequestHandler = async ({ request, locals }) => { const userId = locals.user!.id; const connectionId = request.headers.get(STORAGE_CONNECTION_ID_HEADER); if (!connectionId) { - return json({ error: 'Missing storage connection ID' }, { status: 400 }); + error(400, 'Missing storage connection ID'); } const body = AddConnectionBucketBodySchema.safeParse(await request.json().catch(() => null)); if (!body.success) { - return json({ error: 'Bucket name is required' }, { status: 400 }); + error(400, 'Bucket name is required'); } const { bucket } = body.data; @@ -93,7 +93,7 @@ export const PATCH: RequestHandler = async ({ request, locals }) => { .limit(1); if (rows.length === 0) { - return json({ error: 'Connection not found' }, { status: 404 }); + error(404, 'Connection not found'); } const existing = (rows[0].additionalBuckets as string[]) ?? []; diff --git a/src/routes/(app)/api/storage/copy/job/[jobId]/+server.ts b/src/routes/(app)/api/storage/copy/job/[jobId]/+server.ts index 0bdd2a39..30d6dd69 100644 --- a/src/routes/(app)/api/storage/copy/job/[jobId]/+server.ts +++ b/src/routes/(app)/api/storage/copy/job/[jobId]/+server.ts @@ -7,15 +7,13 @@ export const GET: RequestHandler = async ({ params, locals }) => { const { jobId } = params; if (!z.uuid().safeParse(jobId).success) throw error(400, 'Invalid job ID'); const job = getJob(jobId, locals.user?.id ?? 'anonymous'); - if (!job) { - return json({ status: 'not_found' }, { status: 404 }); - } + if (!job) throw error(404, 'Job not found'); return json(job); }; export const DELETE: RequestHandler = async ({ params, locals }) => { if (!z.uuid().safeParse(params.jobId).success) throw error(400, 'Invalid job ID'); if (!(await cancelJob(params.jobId, locals.user?.id ?? 'anonymous'))) - return json({ status: 'not_found' }, { status: 404 }); + throw error(404, 'Job not found'); return json({ status: 'cancelled' }); }; diff --git a/src/routes/(app)/api/trino/query/+server.ts b/src/routes/(app)/api/trino/query/+server.ts index e726f836..187ca5fa 100644 --- a/src/routes/(app)/api/trino/query/+server.ts +++ b/src/routes/(app)/api/trino/query/+server.ts @@ -1,4 +1,4 @@ -import { json } from '@sveltejs/kit'; +import { error, json } from '@sveltejs/kit'; import { z } from 'zod'; import { getUserId } from '$lib/server/auth-utils.js'; import { @@ -18,19 +18,19 @@ export const POST: RequestHandler = async ({ request, locals }) => { const client = resolveTrinoClient(userId); if (!client) { - return json({ error: 'No Trino connection configured' }, { status: 400 }); + error(400, 'No Trino connection configured'); } let body: unknown; try { body = await request.json(); } catch { - return json({ error: 'Invalid JSON body' }, { status: 400 }); + error(400, 'Invalid JSON body'); } const parsed = StatementRequestSchema.safeParse(body); if (!parsed.success) { - return json({ error: z.prettifyError(parsed.error) }, { status: 400 }); + error(400, z.prettifyError(parsed.error)); } const user = locals.user?.username ?? 'anonymous'; @@ -53,7 +53,7 @@ export const GET: RequestHandler = async ({ url, locals }) => { const userId = getUserId(locals); const parsed = TabIdSchema.safeParse(url.searchParams.get('tabId')); if (!parsed.success) { - return json({ error: 'Missing or invalid tabId parameter' }, { status: 400 }); + error(400, 'Missing or invalid tabId parameter'); } // Lightweight mode omits rows/columns for completed queries to keep polling payloads small. @@ -69,7 +69,7 @@ export const DELETE: RequestHandler = async ({ url, locals }) => { const userId = getUserId(locals); const parsed = TabIdSchema.safeParse(url.searchParams.get('tabId')); if (!parsed.success) { - return json({ error: 'Missing or invalid tabId parameter' }, { status: 400 }); + error(400, 'Missing or invalid tabId parameter'); } const tabId = parsed.data; diff --git a/src/routes/(app)/api/trino/query/server.test.ts b/src/routes/(app)/api/trino/query/server.test.ts index e60a3160..17ad92e3 100644 --- a/src/routes/(app)/api/trino/query/server.test.ts +++ b/src/routes/(app)/api/trino/query/server.test.ts @@ -56,18 +56,17 @@ describe('POST /api/trino/query', () => { it('returns 400 when no Trino connection is configured', async () => { vi.mocked(resolveTrinoClient).mockReturnValue(null); - const res = await POST(postEvent({ statements: ['SELECT 1'], tabId: TAB_ID })); - - expect(res.status).toBe(400); - expect(await res.json()).toEqual({ error: 'No Trino connection configured' }); + await expect( + POST(postEvent({ statements: ['SELECT 1'], tabId: TAB_ID })) + ).rejects.toMatchObject({ status: 400, body: { message: 'No Trino connection configured' } }); expect(startScript).not.toHaveBeenCalled(); }); it('returns 400 for a body that is not valid JSON', async () => { - const res = await POST(postEvent('{not json')); - - expect(res.status).toBe(400); - expect(await res.json()).toEqual({ error: 'Invalid JSON body' }); + await expect(POST(postEvent('{not json'))).rejects.toMatchObject({ + status: 400, + body: { message: 'Invalid JSON body' } + }); }); it.each([ @@ -76,10 +75,10 @@ describe('POST /api/trino/query', () => { ['a non-UUID tabId', { statements: ['SELECT 1'], tabId: 'tab-1' }], ['a missing tabId', { statements: ['SELECT 1'] }] ])('returns 400 for %s', async (_label, body) => { - const res = await POST(postEvent(body)); - - expect(res.status).toBe(400); - expect((await res.json()).error).toEqual(expect.any(String)); + await expect(POST(postEvent(body))).rejects.toMatchObject({ + status: 400, + body: { message: expect.any(String) } + }); expect(startScript).not.toHaveBeenCalled(); }); @@ -142,9 +141,7 @@ describe('GET /api/trino/query', () => { ['missing', ''], ['invalid', 'tabId=not-a-uuid'] ])('returns 400 when tabId is %s', async (_label, params) => { - const res = await GET(urlEvent(params)); - - expect(res.status).toBe(400); + await expect(GET(urlEvent(params))).rejects.toMatchObject({ status: 400 }); expect(getQuerySnapshots).not.toHaveBeenCalled(); }); @@ -173,9 +170,7 @@ describe('DELETE /api/trino/query', () => { }); it('returns 400 when tabId is invalid', async () => { - const res = await DELETE(urlEvent('tabId=nope')); - - expect(res.status).toBe(400); + await expect(DELETE(urlEvent('tabId=nope'))).rejects.toMatchObject({ status: 400 }); expect(cancelQuery).not.toHaveBeenCalled(); }); diff --git a/src/routes/(app)/trino/query-runner.svelte.ts b/src/routes/(app)/trino/query-runner.svelte.ts index 7c485d56..9f054a15 100644 --- a/src/routes/(app)/trino/query-runner.svelte.ts +++ b/src/routes/(app)/trino/query-runner.svelte.ts @@ -9,6 +9,7 @@ import { import type { SqlStatement } from '$lib/editor/split-statements.js'; import { clearCompletionCache } from '$lib/editor/completion/completion-metadata.js'; import * as m from '$lib/paraglide/messages.js'; +import { readErrorMessage } from '$lib/client/api-error.js'; export interface QueryRunner { readonly state: QueryState; @@ -202,8 +203,7 @@ function createQueryRunner(tabId: string): QueryRunner { }); if (!res.ok) { - const body = await res.json().catch(() => null); - error = body?.error ?? m.trino_query_connection_lost(); + error = (await readErrorMessage(res)) ?? m.trino_query_connection_lost(); state = 'FAILED'; return; } From 551f5cf005c8d0ceff7f5bc6b060351652bcd7ab Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Mon, 26 Oct 2026 12:09:15 +0100 Subject: [PATCH 2/5] feat: add friendly error pages (#359) Errors outside the storage browser fell back to SvelteKit's unstyled default page. Add a shared ErrorPanel with a translated title and description per status, the request ID for unexpected errors, and links back to the dashboard or to retry. Server messages are not shown, since they may carry raw upstream details (#304). Unknown pages now render a 404 inside the app layout through a catch-all route, and unknown API paths answer with a JSON 404 instead of HTML. The auth guard no longer treats (app) routes as public because their path starts with a public prefix, so paths like /auth/login/x still require sign-in. src/error.html is a static fallback for errors thrown in hooks. --- e2e/error-pages.spec.ts | 44 +++++++++++++ messages/de.json | 10 +++ messages/en.json | 10 +++ src/error.html | 31 +++++++++ src/hooks.server.ts | 6 +- src/lib/components/ErrorPanel.svelte | 66 ++++++++++++++++++++ src/lib/components/ErrorPanel.svelte.spec.ts | 37 +++++++++++ src/routes/(app)/+error.svelte | 8 +++ src/routes/(app)/[...path]/+page.ts | 8 +++ src/routes/(app)/api/[...path]/+server.ts | 6 ++ src/routes/+error.svelte | 9 +++ 11 files changed, 234 insertions(+), 1 deletion(-) create mode 100644 e2e/error-pages.spec.ts create mode 100644 src/error.html create mode 100644 src/lib/components/ErrorPanel.svelte create mode 100644 src/lib/components/ErrorPanel.svelte.spec.ts create mode 100644 src/routes/(app)/+error.svelte create mode 100644 src/routes/(app)/[...path]/+page.ts create mode 100644 src/routes/(app)/api/[...path]/+server.ts create mode 100644 src/routes/+error.svelte diff --git a/e2e/error-pages.spec.ts b/e2e/error-pages.spec.ts new file mode 100644 index 00000000..e3877ce4 --- /dev/null +++ b/e2e/error-pages.spec.ts @@ -0,0 +1,44 @@ +import { test, expect } from './support/fixtures.js'; +import { waitForHydration } from './support/helpers'; + +test.describe('Error pages', () => { + test.use({ locale: 'en-US' }); + + test('an unknown page shows a readable 404 inside the app layout', async ({ page }) => { + const response = await page.goto('/does-not-exist'); + expect(response?.status()).toBe(404); + await waitForHydration(page); + + const main = page.locator('main'); + await expect(main.getByRole('heading', { name: /Page not found/ })).toBeVisible(); + await expect(page.getByRole('navigation', { name: 'Main navigation' })).toBeVisible(); + + await main.getByRole('link', { name: 'Go to dashboard' }).click(); + await expect(page).toHaveURL('/'); + }); + + test('an unknown API path answers with a JSON error', async ({ page }) => { + const res = await page.request.get('/api/does-not-exist'); + + expect(res.status()).toBe(404); + expect(await res.json()).toEqual({ message: 'Not Found' }); + }); + + test.describe('signed out', () => { + test.use({ storageState: { cookies: [], origins: [] } }); + + test('unknown pages still require sign-in', async ({ page }) => { + for (const path of ['/does-not-exist', '/auth/login/does-not-exist']) { + await page.goto(path); + await expect(page).toHaveURL(/\/auth\/login\?redirectTo=/); + } + }); + + test('unknown API paths still answer with a JSON 401', async ({ page }) => { + const res = await page.request.get('/api/does-not-exist'); + + expect(res.status()).toBe(401); + expect(await res.json()).toEqual({ message: 'Authentication required' }); + }); + }); +}); diff --git a/messages/de.json b/messages/de.json index a08e2e08..7f8bc76a 100644 --- a/messages/de.json +++ b/messages/de.json @@ -19,6 +19,16 @@ "sidebar_favourites": "Favoriten", "sidebar_tools": "Werkzeuge", "skip_to_content": "Zum Hauptinhalt springen", + "error_status": "Fehler {status}", + "error_not_found_title": "Seite nicht gefunden", + "error_not_found_desc": "Die gesuchte Seite existiert nicht oder wurde verschoben.", + "error_forbidden_title": "Zugriff verweigert", + "error_forbidden_desc": "Sie haben keine Berechtigung, diese Seite anzuzeigen.", + "error_generic_title": "Etwas ist schiefgelaufen", + "error_generic_desc": "Ein unerwarteter Fehler ist aufgetreten. Bitte versuchen Sie es später erneut.", + "error_request_id": "Anfrage-ID:", + "error_back_to_dashboard": "Zum Dashboard", + "error_retry": "Erneut versuchen", "header_open_nav": "Navigation öffnen", "header_close_nav": "Navigation schließen", "header_user_menu": "Benutzermenü", diff --git a/messages/en.json b/messages/en.json index 8bd19461..b5c65c9c 100644 --- a/messages/en.json +++ b/messages/en.json @@ -19,6 +19,16 @@ "sidebar_favourites": "Favourites", "sidebar_tools": "Tools", "skip_to_content": "Skip to main content", + "error_status": "Error {status}", + "error_not_found_title": "Page not found", + "error_not_found_desc": "The page you are looking for does not exist or has been moved.", + "error_forbidden_title": "Access denied", + "error_forbidden_desc": "You do not have permission to view this page.", + "error_generic_title": "Something went wrong", + "error_generic_desc": "An unexpected error occurred. Please try again later.", + "error_request_id": "Request ID:", + "error_back_to_dashboard": "Go to dashboard", + "error_retry": "Try again", "header_open_nav": "Open navigation", "header_close_nav": "Close navigation", "header_user_menu": "User menu", diff --git a/src/error.html b/src/error.html new file mode 100644 index 00000000..2142fc0b --- /dev/null +++ b/src/error.html @@ -0,0 +1,31 @@ + + + + + + %sveltekit.status% | Stackable + + + + +
+

%sveltekit.status%

+

%sveltekit.error.message%

+

Stackable

+
+ + diff --git a/src/hooks.server.ts b/src/hooks.server.ts index 086772ae..b5265b0c 100644 --- a/src/hooks.server.ts +++ b/src/hooks.server.ts @@ -55,7 +55,11 @@ const handleAuthGuard: Handle = async ({ event, resolve }) => { event.locals.user = session?.user ?? null; event.locals.session = session?.session ?? null; - const isPublic = PUBLIC_PATHS.some((p) => event.url.pathname.startsWith(p)); + // Routes in the (app) group are never public, even when their path starts + // with a public prefix (e.g. `/auth/login/x` reaching the catch-all page). + const isPublic = + !event.route.id?.startsWith('/(app)') && + PUBLIC_PATHS.some((p) => event.url.pathname.startsWith(p)); if (!isPublic && !event.locals.user) { event.locals.logger.debug({ path: event.url.pathname }, 'Rejecting unauthenticated request'); return unauthenticatedResponse(event.url); diff --git a/src/lib/components/ErrorPanel.svelte b/src/lib/components/ErrorPanel.svelte new file mode 100644 index 00000000..edb825d3 --- /dev/null +++ b/src/lib/components/ErrorPanel.svelte @@ -0,0 +1,66 @@ + + +
+ + +
+ + + {m.error_status({ status })} + + {title} + +

{description}

+ {#if requestId} +

+ {m.error_request_id()} {requestId} +

+ {/if} +
+ +
+ {m.error_back_to_dashboard()} + {#if status !== 404} + + {/if} +
+
diff --git a/src/lib/components/ErrorPanel.svelte.spec.ts b/src/lib/components/ErrorPanel.svelte.spec.ts new file mode 100644 index 00000000..8970c90f --- /dev/null +++ b/src/lib/components/ErrorPanel.svelte.spec.ts @@ -0,0 +1,37 @@ +import { page } from 'vitest/browser'; +import { describe, expect, it, vi } from 'vitest'; +import { render } from 'vitest-browser-svelte'; + +vi.mock('$app/paths', () => ({ resolve: () => '/' })); + +import ErrorPanel from './ErrorPanel.svelte'; + +describe('ErrorPanel', () => { + it.each([ + [404, 'Page not found'], + [403, 'Access denied'], + [500, 'Something went wrong'], + [502, 'Something went wrong'] + ])('shows a readable title for %i', async (status, title) => { + render(ErrorPanel, { status }); + + const heading = page.getByRole('heading'); + await expect.element(heading).toHaveTextContent(`Error ${status}`); + await expect.element(heading).toHaveTextContent(title); + }); + + it('shows the request ID of an unexpected error', async () => { + render(ErrorPanel, { status: 500, requestId: 'req-123' }); + + await expect.element(page.getByText('req-123')).toBeInTheDocument(); + }); + + it('offers a retry for server errors but not for missing pages', async () => { + const { rerender } = render(ErrorPanel, { status: 500 }); + await expect.element(page.getByRole('button', { name: 'Try again' })).toBeInTheDocument(); + + await rerender({ status: 404 }); + await expect.element(page.getByRole('button', { name: 'Try again' })).not.toBeInTheDocument(); + await expect.element(page.getByRole('link', { name: 'Go to dashboard' })).toBeInTheDocument(); + }); +}); diff --git a/src/routes/(app)/+error.svelte b/src/routes/(app)/+error.svelte new file mode 100644 index 00000000..133e7c9c --- /dev/null +++ b/src/routes/(app)/+error.svelte @@ -0,0 +1,8 @@ + + +
+ +
diff --git a/src/routes/(app)/[...path]/+page.ts b/src/routes/(app)/[...path]/+page.ts new file mode 100644 index 00000000..f1cdb4b0 --- /dev/null +++ b/src/routes/(app)/[...path]/+page.ts @@ -0,0 +1,8 @@ +import { error } from '@sveltejs/kit'; +import type { PageLoad } from './$types'; + +// Unknown pages render inside the app layout (with navigation) instead of the +// bare root error page. The client router now resolves every unknown path to +// this page, so plain links to `+server.ts` endpoints need `download`, +// `data-sveltekit-reload` or `rel="external"`. +export const load: PageLoad = () => error(404, 'Not Found'); diff --git a/src/routes/(app)/api/[...path]/+server.ts b/src/routes/(app)/api/[...path]/+server.ts new file mode 100644 index 00000000..635ec24b --- /dev/null +++ b/src/routes/(app)/api/[...path]/+server.ts @@ -0,0 +1,6 @@ +import { error } from '@sveltejs/kit'; +import type { RequestHandler } from './$types'; + +// Without this, unknown API paths fall through to the page catch-all and get +// an HTML error page instead of a JSON body. +export const fallback: RequestHandler = () => error(404, 'Not Found'); diff --git a/src/routes/+error.svelte b/src/routes/+error.svelte new file mode 100644 index 00000000..f45703d4 --- /dev/null +++ b/src/routes/+error.svelte @@ -0,0 +1,9 @@ + + + +
+ +
From 48efc55111ef429d5ad8f83cf4ad859cd4ee0459 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Mon, 26 Oct 2026 12:18:53 +0100 Subject: [PATCH 3/5] fix: show readable errors in catalogue, preview and storage views (#359) - Catalogue browser: show the server's message instead of the raw JSON body, and a translated message when the server cannot be reached. - Preview: the 403/404 messages were never shown, because the storage API already throws a StorageError for non-OK responses. Map the error code in the catch and drop the unreachable !res.ok branches, also in DirectoryDetails along with its unused message key. - directory-size: send the message of SvelteKit's HttpError, which is not an Error instance, instead of "Unknown error". - Storage layout: a failed bucket fetch for the active connection now shows an error above the connect form instead of silently looking disconnected. - Storage and settings connection actions return translated errors, and switching to a saved connection that fails now says why. The settings page shows action errors at all. --- e2e/storage/connection.spec.ts | 17 +++++ e2e/storage/connections-management.spec.ts | 20 ++++++ e2e/trino/catalog-browser.spec.ts | 21 ++++++ messages/de.json | 4 +- messages/en.json | 4 +- .../components/catalog/CatalogBrowser.svelte | 13 ++-- .../storage/landing/StorageConnectForm.svelte | 2 +- .../storage/modals/PreviewModal.svelte | 31 +++----- .../modals/PreviewModal.svelte.spec.ts | 70 ++++++------------- .../modals/details/DirectoryDetails.svelte | 6 -- .../api/storage/directory-size/+server.ts | 10 ++- .../api/storage/directory-size/server.test.ts | 14 ++++ .../settings/connections/+page.server.ts | 3 +- .../(app)/settings/connections/+page.svelte | 5 ++ src/routes/(app)/storage/+layout.ts | 9 ++- src/routes/(app)/storage/+page.server.ts | 50 ++++++------- src/routes/(app)/storage/+page.svelte | 7 +- src/routes/(app)/storage/page.server.test.ts | 25 ++++++- 18 files changed, 193 insertions(+), 118 deletions(-) diff --git a/e2e/storage/connection.spec.ts b/e2e/storage/connection.spec.ts index 2ce6c8f7..441e65f8 100644 --- a/e2e/storage/connection.spec.ts +++ b/e2e/storage/connection.spec.ts @@ -46,6 +46,23 @@ test.describe('Storage S3 — Connection', () => { await expect(page.getByText('Access denied — check your credentials.')).toBeVisible(); }); + test('explains why an active connection cannot be opened', async ({ page }) => { + const credentials = requireGarageCredentials(); + + await connectToStorage(page, credentials); + await expect(page.locator('main').getByRole('heading', { name: 'Buckets' })).toBeVisible(); + + await page.route('**/api/storage/buckets', (route) => + route.fulfill({ status: 502, json: { message: 'Storage error: unreachable' } }) + ); + await page.reload(); + + await expect(page.getByRole('heading', { name: 'Connect to storage' })).toBeVisible(); + await expect( + page.getByText('Could not connect to the selected storage — the endpoint may be unreachable.') + ).toBeVisible(); + }); + test('disconnects from Garage S3', async ({ page }) => { const credentials = requireGarageCredentials(); diff --git a/e2e/storage/connections-management.spec.ts b/e2e/storage/connections-management.spec.ts index d39d5aa9..356dc676 100644 --- a/e2e/storage/connections-management.spec.ts +++ b/e2e/storage/connections-management.spec.ts @@ -247,4 +247,24 @@ test.describe('Storage — Connections management', () => { await waitForHydration(page); await expect(page).toHaveURL('/settings/connections'); }); + + test('shows a translated error when deleting a connection fails', async ({ page }) => { + await page.goto('/settings/connections'); + await waitForHydration(page); + + // The UI only submits valid IDs; post a malformed one directly. + await page.evaluate(() => { + const form = document.createElement('form'); + form.method = 'POST'; + form.action = '?/deleteConnection'; + const input = document.createElement('input'); + input.name = 'connectionId'; + input.value = 'not-a-uuid'; + form.append(input); + document.body.append(form); + form.submit(); + }); + + await expect(page.getByRole('alert')).toHaveText('The selected connection is invalid.'); + }); }); diff --git a/e2e/trino/catalog-browser.spec.ts b/e2e/trino/catalog-browser.spec.ts index 75ce3581..cf662aed 100644 --- a/e2e/trino/catalog-browser.spec.ts +++ b/e2e/trino/catalog-browser.spec.ts @@ -24,6 +24,27 @@ test.describe('Catalog browser', () => { await expect(browser.getByRole('button', { name: 'system' })).toBeVisible(); }); + test('shows a readable message when loading catalogues fails', async ({ page }) => { + await ensureCatalogBrowserOpen(page); + const browser = page.getByRole('navigation', { name: 'Catalog browser' }); + await expect(browser.getByRole('button', { name: 'tpch' })).toBeVisible(); + + await page.route('**/api/trino/catalog?*', (route) => + route.fulfill({ status: 502, json: { message: 'Trino is not reachable' } }) + ); + await page.getByRole('button', { name: 'Refresh catalog' }).click(); + + await expect(browser.getByText('Failed to load catalog')).toBeVisible(); + await expect(browser.getByText('Trino is not reachable')).toBeVisible(); + await expect(browser.getByText('{')).toHaveCount(0); + + await page.unroute('**/api/trino/catalog?*'); + await page.route('**/api/trino/catalog?*', (route) => route.abort()); + await page.getByRole('button', { name: 'Refresh catalog' }).click(); + + await expect(browser.getByText('The server could not be reached.')).toBeVisible(); + }); + test('catalog browser can be toggled', async ({ page }) => { await ensureCatalogBrowserOpen(page); diff --git a/messages/de.json b/messages/de.json index 7f8bc76a..198fde3f 100644 --- a/messages/de.json +++ b/messages/de.json @@ -29,6 +29,7 @@ "error_request_id": "Anfrage-ID:", "error_back_to_dashboard": "Zum Dashboard", "error_retry": "Erneut versuchen", + "error_server_unreachable": "Der Server ist nicht erreichbar. Prüfen Sie Ihre Netzwerkverbindung und versuchen Sie es erneut.", "header_open_nav": "Navigation öffnen", "header_close_nav": "Navigation schließen", "header_user_menu": "Benutzermenü", @@ -182,6 +183,8 @@ "storage_connect_error_server_error": "Das Speicher-Backend hat einen Fehler gemeldet — Endpunkt und Zugangsdaten prüfen.", "storage_connect_error_network": "Speicher-Backend nicht erreichbar — Host und Port prüfen.", "storage_connect_error_hdfs": "HDFS-Verbindungen werden noch nicht unterstützt.", + "storage_connect_error_invalid_connection": "Die ausgewählte Verbindung ist ungültig.", + "storage_connect_error_connection_not_found": "Die ausgewählte Verbindung existiert nicht mehr.", "storage_connect_saved": "Gespeicherte Verbindungen", "storage_connect_no_saved": "Noch keine gespeicherten Verbindungen", "storage_connect_forget": "Löschen", @@ -639,7 +642,6 @@ "storage_details_error_fetch_file": "Dateidetails konnten nicht abgerufen werden ({status})", "storage_details_error_fetch_bucket": "Bucket-Details konnten nicht abgerufen werden ({status})", "storage_details_error_fetch_dir_meta": "Verzeichnismetadaten konnten nicht abgerufen werden ({status})", - "storage_details_error_calc_size": "Verzeichnisgröße konnte nicht berechnet werden ({status})", "storage_details_error_unknown": "Unbekannter Fehler", "storage_details_error_no_body": "Kein Antworttext", "storage_details_unnamed_rule": "(unbenannt)", diff --git a/messages/en.json b/messages/en.json index b5c65c9c..c9404fc0 100644 --- a/messages/en.json +++ b/messages/en.json @@ -29,6 +29,7 @@ "error_request_id": "Request ID:", "error_back_to_dashboard": "Go to dashboard", "error_retry": "Try again", + "error_server_unreachable": "The server could not be reached. Check your network connection and try again.", "header_open_nav": "Open navigation", "header_close_nav": "Close navigation", "header_user_menu": "User menu", @@ -182,6 +183,8 @@ "storage_connect_error_server_error": "The storage backend returned an error — check the endpoint and credentials.", "storage_connect_error_network": "Could not reach the storage backend — check the host and port.", "storage_connect_error_hdfs": "HDFS connections are not yet supported.", + "storage_connect_error_invalid_connection": "The selected connection is invalid.", + "storage_connect_error_connection_not_found": "The selected connection no longer exists.", "storage_connect_saved": "Saved connections", "storage_connect_no_saved": "No saved connections yet", "storage_connect_forget": "Delete", @@ -639,7 +642,6 @@ "storage_details_error_fetch_file": "Failed to fetch file details ({status})", "storage_details_error_fetch_bucket": "Failed to fetch bucket details ({status})", "storage_details_error_fetch_dir_meta": "Failed to fetch directory metadata ({status})", - "storage_details_error_calc_size": "Failed to calculate directory size ({status})", "storage_details_error_unknown": "Unknown error", "storage_details_error_no_body": "No response body", "storage_details_unnamed_rule": "(unnamed)", diff --git a/src/lib/components/catalog/CatalogBrowser.svelte b/src/lib/components/catalog/CatalogBrowser.svelte index 5bcad159..178947c6 100644 --- a/src/lib/components/catalog/CatalogBrowser.svelte +++ b/src/lib/components/catalog/CatalogBrowser.svelte @@ -2,6 +2,7 @@ import { untrack } from 'svelte'; import { SvelteSet } from 'svelte/reactivity'; import * as m from '$lib/paraglide/messages.js'; + import { readErrorMessage } from '$lib/client/api-error.js'; import IconRefresh from 'virtual:icons/material-symbols/refresh'; import CatalogTree from './CatalogTree.svelte'; import type { TreeNode } from './types.js'; @@ -30,12 +31,16 @@ async function fetchLevel(params: Record): Promise { const qs = new URLSearchParams(params).toString(); - const res = await fetch(`/api/trino/catalog?${qs}`); + // Every failure throws an Error whose message can be shown to the user. + const res = await fetch(`/api/trino/catalog?${qs}`).catch(() => { + throw new Error(m.error_server_unreachable()); + }); if (!res.ok) { - const text = await res.text(); - throw new Error(text); + throw new Error((await readErrorMessage(res)) ?? m.error_generic_desc()); } - return res.json(); + return res.json().catch(() => { + throw new Error(m.error_generic_desc()); + }); } async function loadCatalogs() { diff --git a/src/lib/components/storage/landing/StorageConnectForm.svelte b/src/lib/components/storage/landing/StorageConnectForm.svelte index c15ff6b3..f7098b9a 100644 --- a/src/lib/components/storage/landing/StorageConnectForm.svelte +++ b/src/lib/components/storage/landing/StorageConnectForm.svelte @@ -57,7 +57,7 @@ {#if connectError} {/if} diff --git a/src/lib/components/storage/modals/PreviewModal.svelte b/src/lib/components/storage/modals/PreviewModal.svelte index 1aef7bab..1afcec27 100644 --- a/src/lib/components/storage/modals/PreviewModal.svelte +++ b/src/lib/components/storage/modals/PreviewModal.svelte @@ -222,18 +222,6 @@ res = await storage.api.preview({ bucket: activeBucket, key }); } - if (!res.ok) { - const errBody = await res.json().catch(() => ({})); - const msg = - res.status === 403 - ? m.storage_preview_access_denied() - : res.status === 404 - ? m.storage_preview_not_found() - : (errBody?.error ?? m.storage_preview_error_desc()); - preview = { kind: 'error', message: msg }; - return; - } - const contentType = (res.headers.get('Content-Type') ?? 'application/octet-stream') .split(';')[0] .trim(); @@ -412,8 +400,15 @@ }; editorText = text; originalText = text; - } catch { - preview = { kind: 'error', message: m.storage_preview_error_desc() }; + } catch (err) { + const code = err instanceof StorageError ? err.code : null; + const message = + code === 'access_denied' + ? m.storage_preview_access_denied() + : code === 'not_found' + ? m.storage_preview_not_found() + : m.storage_preview_error_desc(); + preview = { kind: 'error', message }; } } @@ -746,10 +741,6 @@ data: true }); - if (!res.ok) { - throw new Error('Failed to fetch parquet chunk'); - } - const { rows } = await readNdjsonStream(res, onColumn); return rows; } @@ -766,10 +757,6 @@ data: true }); - if (!res.ok) { - throw new Error('Failed to fetch CSV chunk'); - } - const { rows, totalRows } = await readCsvNdjsonStream(res); csvTotalRows = totalRows; return rows; diff --git a/src/lib/components/storage/modals/PreviewModal.svelte.spec.ts b/src/lib/components/storage/modals/PreviewModal.svelte.spec.ts index 308fa75e..2c269268 100644 --- a/src/lib/components/storage/modals/PreviewModal.svelte.spec.ts +++ b/src/lib/components/storage/modals/PreviewModal.svelte.spec.ts @@ -14,21 +14,20 @@ vi.mock('$app/paths', () => ({ })); // Mock storage context — provide an API that delegates to the global fetch -vi.mock('$lib/storage/context.js', () => ({ - getStorageState: () => ({ - get api() { - return { - preview: (...args: unknown[]) => - (globalThis.fetch as typeof fetch)(...(args as Parameters)), - archiveExtract: (...args: unknown[]) => - (globalThis.fetch as typeof fetch)(...(args as Parameters)), - saveText: (...args: unknown[]) => - (globalThis.fetch as typeof fetch)(...(args as Parameters)) - }; - }, - bucket: 'test-bucket' - }) -})); +// Like the real API, non-OK responses throw a StorageError. +vi.mock('$lib/storage/context.js', async () => { + const { createStorageFetch } = await import('$lib/storage/storage-fetch.js'); + const storageFetch = createStorageFetch(() => 'conn-1'); + const call = (...args: unknown[]) => storageFetch(...(args as Parameters)); + return { + getStorageState: () => ({ + get api() { + return { preview: call, archiveExtract: call, saveText: call }; + }, + bucket: 'test-bucket' + }) + }; +}); const defaultProps = { open: true, @@ -716,42 +715,15 @@ describe('PreviewModal basics', () => { await expect.element(page.getByRole('alert')).toBeInTheDocument(); }); - it('should show access denied for 403', async () => { - vi.stubGlobal('fetch', vi.fn().mockResolvedValue(new Response('{}', { status: 403 }))); - render(PreviewModal, defaultProps); - - await expect.element(page.getByRole('alert')).toBeInTheDocument(); - }); - - it('should show not found for 404', async () => { - vi.stubGlobal('fetch', vi.fn().mockResolvedValue(new Response('{}', { status: 404 }))); - render(PreviewModal, defaultProps); - - await expect.element(page.getByRole('alert')).toBeInTheDocument(); - }); - - it('should show generic error for other status codes', async () => { - vi.stubGlobal( - 'fetch', - vi - .fn() - .mockResolvedValue( - new Response(JSON.stringify({ error: 'Server is down' }), { status: 500 }) - ) - ); - render(PreviewModal, defaultProps); - - await expect.element(page.getByRole('alert')).toBeInTheDocument(); - }); - - it('should handle non-JSON error response body', async () => { - vi.stubGlobal( - 'fetch', - vi.fn().mockResolvedValue(new Response('Not JSON at all', { status: 500 })) - ); + it.each([ + [403, '{}', 'You do not have permission to preview this file.'], + [404, '{}', 'The file could not be found.'], + [500, JSON.stringify({ message: 'Storage error: SlowDown' }), 'An error occurred'] + ])('should show a readable message for a %i response', async (status, body, text) => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(new Response(body, { status }))); render(PreviewModal, defaultProps); - await expect.element(page.getByRole('alert')).toBeInTheDocument(); + await expect.element(page.getByRole('alert')).toHaveTextContent(text); }); }); diff --git a/src/lib/components/storage/modals/details/DirectoryDetails.svelte b/src/lib/components/storage/modals/details/DirectoryDetails.svelte index e8ac777e..da8039e1 100644 --- a/src/lib/components/storage/modals/details/DirectoryDetails.svelte +++ b/src/lib/components/storage/modals/details/DirectoryDetails.svelte @@ -78,12 +78,6 @@ try { const res = await storage.api.directorySize({ bucket, prefix, signal: request.signal }); - if (!res.ok) { - error = m.storage_details_error_calc_size({ status: res.status }); - calculating = false; - return; - } - const reader = res.body?.getReader(); if (!reader) { error = m.storage_details_error_no_body(); diff --git a/src/routes/(app)/api/storage/directory-size/+server.ts b/src/routes/(app)/api/storage/directory-size/+server.ts index c8959249..c328bc4f 100644 --- a/src/routes/(app)/api/storage/directory-size/+server.ts +++ b/src/routes/(app)/api/storage/directory-size/+server.ts @@ -1,4 +1,4 @@ -import { error } from '@sveltejs/kit'; +import { error, isHttpError } from '@sveltejs/kit'; import type { DirectorySizeEvent } from '$lib/storage/details-types.js'; import { DIRECTORY_SIZE_MAX_KEYS, @@ -64,7 +64,13 @@ export const GET: RequestHandler = async (event) => { } storageDirectorySizeCalculationsTotal.inc({ outcome: 'error' }); log.warn({ err, bucket, prefix }, 'directory size calculation failed'); - send({ type: 'error', message: err instanceof Error ? err.message : 'Unknown error' }); + // SvelteKit's HttpError (e.g. a mapped S3 403) is not an Error instance. + const message = isHttpError(err) + ? err.body.message + : err instanceof Error + ? err.message + : 'Unknown error'; + send({ type: 'error', message }); controller.close(); return; } diff --git a/src/routes/(app)/api/storage/directory-size/server.test.ts b/src/routes/(app)/api/storage/directory-size/server.test.ts index d0ab2167..a725303e 100644 --- a/src/routes/(app)/api/storage/directory-size/server.test.ts +++ b/src/routes/(app)/api/storage/directory-size/server.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { error } from '@sveltejs/kit'; import type { DirectorySizeEvent } from '$lib/storage/details-types.js'; import type { ProgressiveListOptions } from '$lib/server/storage/provider.js'; @@ -66,6 +67,19 @@ describe('GET /api/storage/directory-size', () => { expect(events).toEqual([{ type: 'error', message: 'Access Denied' }]); }); + it('sends the message of a mapped storage error', async () => { + let httpError: unknown; + try { + error(403, 'Access denied to bucket'); + } catch (err) { + httpError = err; + } + mockProvider.listAllKeysProgressively.mockRejectedValue(httpError); + + const events = await readEvents(await GET(mockEvent('p/'))); + expect(events).toEqual([{ type: 'error', message: 'Access denied to bucket' }]); + }); + it('aborts the listing when the client goes away', async () => { let listingSignal: AbortSignal | undefined; mockProvider.listAllKeysProgressively.mockImplementation( diff --git a/src/routes/(app)/settings/connections/+page.server.ts b/src/routes/(app)/settings/connections/+page.server.ts index 638de09f..e9680a40 100644 --- a/src/routes/(app)/settings/connections/+page.server.ts +++ b/src/routes/(app)/settings/connections/+page.server.ts @@ -11,6 +11,7 @@ import { userStorageConnections } from '$lib/server/schema.js'; import { decrypt } from '$lib/server/storage/encryption.js'; import { storageEncryptionKey } from '$lib/server/storage/encryption-key.js'; import type { ConnectionListItem } from '$lib/storage/connection-store.svelte.js'; +import * as m from '$lib/paraglide/messages.js'; export const load: PageServerLoad = async ({ locals }) => { locals.logger.debug('loading storage connections management page'); @@ -54,7 +55,7 @@ export const actions: Actions = { const form = await superValidate(request, zod(ConnectionIdSchema)); if (!form.valid) { - return fail(400, { error: 'Invalid connection ID' }); + return fail(400, { error: m.storage_connect_error_invalid_connection() }); } const { connectionId } = form.data; diff --git a/src/routes/(app)/settings/connections/+page.svelte b/src/routes/(app)/settings/connections/+page.svelte index 2845e9d9..596d5147 100644 --- a/src/routes/(app)/settings/connections/+page.svelte +++ b/src/routes/(app)/settings/connections/+page.svelte @@ -13,6 +13,7 @@ import type { SavedConnection } from '$lib/storage/connection-id-header.js'; const connections = $derived(page.data.connections ?? []); + const actionError = $derived((page.form as { error?: string } | null)?.error ?? null); // Context menu state let menuConn: ConnectionListItem | null = $state(null); @@ -90,6 +91,10 @@

{m.storage_connections_title()}

{m.storage_connections_subtitle()}

+ {#if actionError} + + {/if} + {#if connections.length === 0}

{m.storage_connections_empty()}

{:else} diff --git a/src/routes/(app)/storage/+layout.ts b/src/routes/(app)/storage/+layout.ts index c82d37b4..705b3fbb 100644 --- a/src/routes/(app)/storage/+layout.ts +++ b/src/routes/(app)/storage/+layout.ts @@ -6,7 +6,9 @@ import type { LayoutLoad } from './$types'; const DISCONNECTED = { connected: false, buckets: [] as string[], - connectionType: null + connectionType: null, + /** Why the active connection could not be opened; the connect form shows it. */ + bucketsError: null as 'access_denied' | 'unreachable' | null }; /** @@ -65,6 +67,9 @@ export const load: LayoutLoad = async ({ fetch, data }) => { if (!res.ok) { return { ...DISCONNECTED, + // 401: the session holds no usable connection, which is not an error. + bucketsError: + res.status === 401 ? null : res.status === 403 ? 'access_denied' : 'unreachable', connections, hydrating: false as const, hasActiveConnection: false as const @@ -76,6 +81,7 @@ export const load: LayoutLoad = async ({ fetch, data }) => { connected: true, buckets, connectionType: 's3', + bucketsError: null, connections, hydrating: false as const, hasActiveConnection: true as const @@ -83,6 +89,7 @@ export const load: LayoutLoad = async ({ fetch, data }) => { } catch { return { ...DISCONNECTED, + bucketsError: 'unreachable', connections, hydrating: false as const, hasActiveConnection: false as const diff --git a/src/routes/(app)/storage/+page.server.ts b/src/routes/(app)/storage/+page.server.ts index 73d2e4eb..c7a751d0 100644 --- a/src/routes/(app)/storage/+page.server.ts +++ b/src/routes/(app)/storage/+page.server.ts @@ -28,6 +28,23 @@ import * as m from '$lib/paraglide/messages.js'; type ConnectionData = Infer; type ConnectionInput = InferIn; +/** Translated reason why a storage connection test failed. */ +function connectErrorMessage(err: unknown): string { + if (isHttpError(err)) { + if (err.status === 403) return m.storage_connect_error_access_denied(); + if (err.status === 404) return m.storage_connect_error_not_found(); + if (err.status === 502) return m.storage_connect_error_server_error(); + return m.storage_connect_error(); + } + if (err instanceof Error) { + const e = (err.message ?? '').toLowerCase(); + if (/econnrefused|enotfound|eai_again|etimedout|network/.test(e)) { + return m.storage_connect_error_network(); + } + } + return m.storage_connect_error(); +} + export const load: PageServerLoad = async ({ locals }) => { const log = locals.logger; const connectionForm = await superValidate< @@ -106,30 +123,7 @@ export const actions: Actions = { log.info({ storage_type: type }, 'user storage connection verified'); } catch (err) { log.warn({ err }, 'storage connection test failed'); - - let msg: string; - if (isHttpError(err)) { - if (err.status === 403) { - msg = m.storage_connect_error_access_denied(); - } else if (err.status === 404) { - msg = m.storage_connect_error_not_found(); - } else if (err.status === 502) { - msg = m.storage_connect_error_server_error(); - } else { - msg = m.storage_connect_error(); - } - } else if (err instanceof Error) { - const e = (err.message ?? '').toLowerCase(); - if (/econnrefused|enotfound|eai_again|etimedout|network/.test(e)) { - msg = m.storage_connect_error_network(); - } else { - msg = m.storage_connect_error(); - } - } else { - msg = m.storage_connect_error(); - } - - return message(form, { type: 'error', message: msg }, { status: 400 }); + return message(form, { type: 'error', message: connectErrorMessage(err) }, { status: 400 }); } const id = await saveConnection(locals.user!.id, config); @@ -155,7 +149,7 @@ export const actions: Actions = { const form = await superValidate(request, zod(ConnectionIdSchema)); if (!form.valid) { - return fail(400, { error: 'Invalid connection ID' }); + return fail(400, { error: m.storage_connect_error_invalid_connection() }); } const { connectionId } = form.data; @@ -163,7 +157,7 @@ export const actions: Actions = { const config = await getConnectionForUser(userId, connectionId); if (!config) { - return fail(400, { error: 'Connection not found' }); + return fail(400, { error: m.storage_connect_error_connection_not_found() }); } try { @@ -171,7 +165,7 @@ export const actions: Actions = { log.info({ connectionId }, 'user switched storage connection'); } catch (err) { log.warn({ err, connectionId }, 'storage connection test failed on use'); - return fail(400, { error: 'Could not connect — check the endpoint and credentials.' }); + return fail(400, { error: connectErrorMessage(err) }); } await auth.api.updateSession({ @@ -187,7 +181,7 @@ export const actions: Actions = { const form = await superValidate(request, zod(ConnectionIdSchema)); if (!form.valid) { - return fail(400, { error: 'Invalid connection ID' }); + return fail(400, { error: m.storage_connect_error_invalid_connection() }); } const { connectionId } = form.data; diff --git a/src/routes/(app)/storage/+page.svelte b/src/routes/(app)/storage/+page.svelte index ad87eb38..272a1b7a 100644 --- a/src/routes/(app)/storage/+page.svelte +++ b/src/routes/(app)/storage/+page.svelte @@ -25,7 +25,12 @@ let addBucketOpen = $state(false); const connectError = $derived( - (page.form as { error?: string } | null)?.error ?? null + (page.form as { error?: string } | null)?.error ?? + (data.bucketsError === 'access_denied' + ? m.storage_connect_error_access_denied() + : data.bucketsError === 'unreachable' + ? m.storage_connect_error_unreachable() + : null) ); let savedTabs = $state(null); diff --git a/src/routes/(app)/storage/page.server.test.ts b/src/routes/(app)/storage/page.server.test.ts index fe5c3d98..8d25dc28 100644 --- a/src/routes/(app)/storage/page.server.test.ts +++ b/src/routes/(app)/storage/page.server.test.ts @@ -203,7 +203,30 @@ describe('storage page actions', () => { locals: mockLocals() } as unknown as Parameters[0]); - expect(result).toMatchObject({ status: 400, data: { error: 'Connection not found' } }); + expect(result).toMatchObject({ + status: 400, + data: { error: 'The selected connection no longer exists.' } + }); + }); + + it('use: explains why the selected connection cannot be opened', async () => { + vi.mocked(superValidate).mockResolvedValue({ + valid: true, + data: { connectionId: 'saved-connection' } + } as unknown as Awaited>); + mockGetConnectionForUser.mockResolvedValue(validFormData); + mockConnectionProvider.listContainers.mockRejectedValue(new Error('getaddrinfo ENOTFOUND')); + + const result = await actions.use({ + request: new Request('http://localhost', { method: 'POST' }), + locals: mockLocals() + } as unknown as Parameters[0]); + + expect(result).toMatchObject({ + status: 400, + data: { error: 'Could not reach the storage backend — check the host and port.' } + }); + expect(mockUpdateSession).not.toHaveBeenCalled(); }); it('use: sets the selected connection as active and redirects', async () => { From 9298da71d6d47e3e9e3f3b682472e0cc7cafd64a Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Mon, 26 Oct 2026 13:02:52 +0100 Subject: [PATCH 4/5] fix: address review findings on error handling (#359) - readErrorMessage ignores 401s and unexpected errors, whose bodies only carry a generic English text, so callers fall back to translated messages. A failed directory size calculation and a failed preview download show translated messages too. - The auth guard matches public routes by route id instead of path prefix, so lookalike paths such as /metricsfoo require sign-in. - The root error page does not link failing sign-in pages to the dashboard, which would redirect straight back to sign-in. - ErrorPanel replaces StorageErrorPanel, with optional title, description and actions. - The 404 page uses "Page not found" as page title and heading. - Type the storage layout's buckets error reason. --- e2e/error-pages.spec.ts | 11 ++++ e2e/storage/details.spec.ts | 27 +++++++++ messages/de.json | 1 + messages/en.json | 1 + src/hooks.server.ts | 11 ++-- src/lib/client/api-error.spec.ts | 12 ++-- src/lib/client/api-error.ts | 17 +++--- src/lib/components/ErrorPanel.svelte | 33 +++++++---- .../storage/modals/PreviewModal.svelte | 17 +++--- .../modals/PreviewModal.svelte.spec.ts | 20 ++++++- .../modals/details/DirectoryDetails.svelte | 8 +-- .../storage/shared/StorageErrorPanel.svelte | 57 ------------------- .../shared/StorageErrorPanel.svelte.spec.ts | 37 ------------ src/routes/(app)/+layout.svelte | 12 ++-- src/routes/(app)/storage/+error.svelte | 17 +++++- src/routes/(app)/storage/+layout.ts | 18 ++++-- src/routes/(app)/storage/error.svelte.spec.ts | 14 +++++ src/routes/+error.svelte | 19 ++++++- src/routes/error.svelte.spec.ts | 32 +++++++++++ 19 files changed, 208 insertions(+), 156 deletions(-) delete mode 100644 src/lib/components/storage/shared/StorageErrorPanel.svelte delete mode 100644 src/lib/components/storage/shared/StorageErrorPanel.svelte.spec.ts create mode 100644 src/routes/error.svelte.spec.ts diff --git a/e2e/error-pages.spec.ts b/e2e/error-pages.spec.ts index e3877ce4..721096bb 100644 --- a/e2e/error-pages.spec.ts +++ b/e2e/error-pages.spec.ts @@ -9,6 +9,8 @@ test.describe('Error pages', () => { expect(response?.status()).toBe(404); await waitForHydration(page); + await expect(page).toHaveTitle('Page not found | Stackable'); + await expect(page.getByRole('heading', { level: 1 })).toHaveText('Page not found'); const main = page.locator('main'); await expect(main.getByRole('heading', { name: /Page not found/ })).toBeVisible(); await expect(page.getByRole('navigation', { name: 'Main navigation' })).toBeVisible(); @@ -34,6 +36,15 @@ test.describe('Error pages', () => { } }); + test('only the exact public routes skip sign-in', async ({ page }) => { + for (const path of ['/metrics', '/healthz']) { + expect((await page.request.get(path)).status(), path).toBe(200); + } + const lookalike = await page.request.get('/metricsfoo', { maxRedirects: 0 }); + expect(lookalike.status()).toBe(302); + expect(lookalike.headers().location).toMatch(/^\/auth\/login\?redirectTo=/); + }); + test('unknown API paths still answer with a JSON 401', async ({ page }) => { const res = await page.request.get('/api/does-not-exist'); diff --git a/e2e/storage/details.spec.ts b/e2e/storage/details.spec.ts index 8e78b8fb..fcec5179 100644 --- a/e2e/storage/details.spec.ts +++ b/e2e/storage/details.spec.ts @@ -97,6 +97,33 @@ test.describe('Storage S3 — Details Modal', () => { } }); + test('explains a failed size calculation', async ({ page }, testInfo) => { + const credentials = requireGarageCredentials(); + const client = createS3Client(credentials); + const prefix = uniquePrefix(testInfo, 'details-size-error'); + const cleanupKeys = [`${prefix}subdir/file.txt`]; + + try { + await putTextObject(client, credentials.bucket, cleanupKeys[0], 'x'); + // A proxy in front of the app answers with its own HTML error page. + await page.route('**/api/storage/directory-size?*', (route) => + route.fulfill({ status: 502, contentType: 'text/html', body: 'Bad Gateway' }) + ); + + await connectAndOpenPrefix(page, credentials, prefix); + await rowByName(page, 'subdir').click({ button: 'right' }); + await page.getByRole('menuitem', { name: 'Details' }).click(); + + const modal = modalBox(page); + await modal.getByRole('button', { name: 'Calculate directory size' }).click(); + await expect( + modal.getByText('Failed to calculate the directory size. Please try again.') + ).toBeVisible(); + } finally { + await deleteKnownKeys(client, credentials.bucket, cleanupKeys); + } + }); + test('shows partial results when the key limit is reached', async ({ page }, testInfo) => { const credentials = requireGarageCredentials(); const client = createS3Client(credentials); diff --git a/messages/de.json b/messages/de.json index 198fde3f..26cfd63e 100644 --- a/messages/de.json +++ b/messages/de.json @@ -642,6 +642,7 @@ "storage_details_error_fetch_file": "Dateidetails konnten nicht abgerufen werden ({status})", "storage_details_error_fetch_bucket": "Bucket-Details konnten nicht abgerufen werden ({status})", "storage_details_error_fetch_dir_meta": "Verzeichnismetadaten konnten nicht abgerufen werden ({status})", + "storage_details_error_calc_size": "Die Verzeichnisgröße konnte nicht berechnet werden. Bitte versuchen Sie es erneut.", "storage_details_error_unknown": "Unbekannter Fehler", "storage_details_error_no_body": "Kein Antworttext", "storage_details_unnamed_rule": "(unbenannt)", diff --git a/messages/en.json b/messages/en.json index c9404fc0..d2fc91de 100644 --- a/messages/en.json +++ b/messages/en.json @@ -642,6 +642,7 @@ "storage_details_error_fetch_file": "Failed to fetch file details ({status})", "storage_details_error_fetch_bucket": "Failed to fetch bucket details ({status})", "storage_details_error_fetch_dir_meta": "Failed to fetch directory metadata ({status})", + "storage_details_error_calc_size": "Failed to calculate the directory size. Please try again.", "storage_details_error_unknown": "Unknown error", "storage_details_error_no_body": "No response body", "storage_details_unnamed_rule": "(unnamed)", diff --git a/src/hooks.server.ts b/src/hooks.server.ts index b5265b0c..aa317dc0 100644 --- a/src/hooks.server.ts +++ b/src/hooks.server.ts @@ -48,18 +48,17 @@ const handleParaglide: Handle = ({ event, resolve }) => const handleAuth: Handle = ({ event, resolve }) => svelteKitHandler({ event, resolve, auth, building }); -const PUBLIC_PATHS = ['/auth/login', '/auth/logout', '/api/auth', '/metrics', '/healthz']; +// Matched by route id, not path prefix, so `/metricsfoo` or `/auth/login/x` +// (which reaches the (app) catch-all) still require sign-in. `/api/auth/*` is +// answered by better-auth in `handleAuth` before this guard runs. +const PUBLIC_ROUTE_IDS = new Set(['/auth/login', '/auth/logout', '/metrics', '/healthz']); const handleAuthGuard: Handle = async ({ event, resolve }) => { const session = await auth.api.getSession({ headers: event.request.headers }); event.locals.user = session?.user ?? null; event.locals.session = session?.session ?? null; - // Routes in the (app) group are never public, even when their path starts - // with a public prefix (e.g. `/auth/login/x` reaching the catch-all page). - const isPublic = - !event.route.id?.startsWith('/(app)') && - PUBLIC_PATHS.some((p) => event.url.pathname.startsWith(p)); + const isPublic = event.route.id !== null && PUBLIC_ROUTE_IDS.has(event.route.id); if (!isPublic && !event.locals.user) { event.locals.logger.debug({ path: event.url.pathname }, 'Rejecting unauthenticated request'); return unauthenticatedResponse(event.url); diff --git a/src/lib/client/api-error.spec.ts b/src/lib/client/api-error.spec.ts index f3678294..6c0e45ff 100644 --- a/src/lib/client/api-error.spec.ts +++ b/src/lib/client/api-error.spec.ts @@ -9,10 +9,12 @@ describe('readErrorMessage', () => { }); it.each([ - ['an HTML page', ''], - ['an empty body', ''], - ['JSON without a message', '{"error":"nope"}'] - ])('returns null for %s', async (_label, body) => { - expect(await readErrorMessage(new Response(body, { status: 502 }))).toBeNull(); + ['an HTML page', 502, ''], + ['an empty body', 502, ''], + ['JSON without a message', 502, '{"error":"nope"}'], + ['an expired session', 401, '{"message":"Authentication required"}'], + ['an unexpected error', 500, '{"message":"An unexpected error occurred","requestId":"r1"}'] + ])('returns null for %s', async (_label, status, body) => { + expect(await readErrorMessage(new Response(body, { status }))).toBeNull(); }); }); diff --git a/src/lib/client/api-error.ts b/src/lib/client/api-error.ts index 1b5395d1..4477fb09 100644 --- a/src/lib/client/api-error.ts +++ b/src/lib/client/api-error.ts @@ -3,18 +3,19 @@ * * Every API route answers errors with an `App.Error` body (`{ message }`), * produced by SvelteKit's `error()`, `handleError` or the auth guard. Returns - * `null` when the body is empty or not JSON, e.g. a proxy's HTML error page, - * so callers can fall back to a translated message. + * `null` when the message is not worth showing, so callers can fall back to a + * translated message: + * - the body is empty or not JSON, e.g. a proxy's HTML error page; + * - a 401: the session expired and the session-expired modal takes over; + * - an unexpected error: `handleError` only sends a generic English text, + * recognisable by its `requestId`. */ export async function readErrorMessage(res: Response): Promise { + if (res.status === 401) return null; const body: unknown = await res .clone() .json() .catch(() => null); - return body !== null && - typeof body === 'object' && - 'message' in body && - typeof body.message === 'string' - ? body.message - : null; + if (body === null || typeof body !== 'object' || 'requestId' in body) return null; + return 'message' in body && typeof body.message === 'string' ? body.message : null; } diff --git a/src/lib/components/ErrorPanel.svelte b/src/lib/components/ErrorPanel.svelte index edb825d3..878ce031 100644 --- a/src/lib/components/ErrorPanel.svelte +++ b/src/lib/components/ErrorPanel.svelte @@ -1,4 +1,5 @@ - -
- - -
-

- - {m.storage_error_title()} - - - {status} - -

-

- {message} -

-
- -
- - {m.storage_error_back_to_storage()} - - -
-
diff --git a/src/lib/components/storage/shared/StorageErrorPanel.svelte.spec.ts b/src/lib/components/storage/shared/StorageErrorPanel.svelte.spec.ts deleted file mode 100644 index f96158c5..00000000 --- a/src/lib/components/storage/shared/StorageErrorPanel.svelte.spec.ts +++ /dev/null @@ -1,37 +0,0 @@ -import { page } from 'vitest/browser'; -import { describe, expect, it } from 'vitest'; -import { render } from 'vitest-browser-svelte'; -import StorageErrorPanel from './StorageErrorPanel.svelte'; - -describe('StorageErrorPanel', () => { - it('should render the status code as the heading', async () => { - render(StorageErrorPanel, { status: 403, message: 'Access denied' }); - - await expect.element(page.getByRole('heading', { name: '403' })).toBeInTheDocument(); - }); - - it('should render the supplied message', async () => { - render(StorageErrorPanel, { status: 404, message: 'No such bucket: reports' }); - - await expect.element(page.getByText('No such bucket: reports')).toBeInTheDocument(); - }); - - it('should tolerate an empty message', async () => { - render(StorageErrorPanel, { status: 500, message: '' }); - - await expect.element(page.getByRole('heading', { name: '500' })).toBeInTheDocument(); - }); - - it('should offer a link back to storage', async () => { - render(StorageErrorPanel, { status: 403, message: 'Access denied' }); - - const link = page.getByRole('link', { name: /back to storage/i }); - await expect.element(link).toHaveAttribute('href', '/storage'); - }); - - it('should offer a go-back button', async () => { - render(StorageErrorPanel, { status: 403, message: 'Access denied' }); - - await expect.element(page.getByRole('button', { name: /go back/i })).toBeInTheDocument(); - }); -}); diff --git a/src/routes/(app)/+layout.svelte b/src/routes/(app)/+layout.svelte index 662d6665..4543e18e 100644 --- a/src/routes/(app)/+layout.svelte +++ b/src/routes/(app)/+layout.svelte @@ -31,12 +31,16 @@ '/trino': m.page_title_trino }; - let title = $derived( - ( + function pageTitle(): () => string { + // Unknown pages render the 404 of the catch-all route. + if (page.route.id === '/(app)/[...path]') return m.error_not_found_title; + return ( pageTitles[page.url.pathname] ?? (page.url.pathname.startsWith('/storage') ? m.page_title_storage : m.page_title_default) - )() - ); + ); + } + + let title = $derived(pageTitle()()); let mainEl: HTMLElement | undefined = $state(); diff --git a/src/routes/(app)/storage/+error.svelte b/src/routes/(app)/storage/+error.svelte index 1bfe7ef9..72259866 100644 --- a/src/routes/(app)/storage/+error.svelte +++ b/src/routes/(app)/storage/+error.svelte @@ -4,7 +4,7 @@ import IconStorage from 'virtual:icons/material-symbols/storage'; import IconChevronRight from 'virtual:icons/material-symbols/chevron-right'; import * as m from '$lib/paraglide/messages.js'; - import StorageErrorPanel from '$lib/components/storage/shared/StorageErrorPanel.svelte'; + import ErrorPanel from '$lib/components/ErrorPanel.svelte'; function safeDecode(value: string): string { try { @@ -74,5 +74,18 @@ {/if} - + + {#snippet actions()} + + {m.storage_error_back_to_storage()} + + + {/snippet} + diff --git a/src/routes/(app)/storage/+layout.ts b/src/routes/(app)/storage/+layout.ts index 705b3fbb..22e81c7a 100644 --- a/src/routes/(app)/storage/+layout.ts +++ b/src/routes/(app)/storage/+layout.ts @@ -3,12 +3,20 @@ import { STORAGE_CONNECTION_ID_HEADER } from '$lib/storage/connection-id-header. import { connectionStore } from '$lib/storage/connection-store.svelte.js'; import type { LayoutLoad } from './$types'; +/** Why the active connection could not be opened; the connect form shows it. */ +type BucketsError = 'access_denied' | 'unreachable' | null; + +// 401: the session holds no usable connection, which is not an error. +function bucketsErrorFor(status: number): BucketsError { + if (status === 401) return null; + return status === 403 ? 'access_denied' : 'unreachable'; +} + const DISCONNECTED = { connected: false, buckets: [] as string[], connectionType: null, - /** Why the active connection could not be opened; the connect form shows it. */ - bucketsError: null as 'access_denied' | 'unreachable' | null + bucketsError: null as BucketsError }; /** @@ -67,9 +75,7 @@ export const load: LayoutLoad = async ({ fetch, data }) => { if (!res.ok) { return { ...DISCONNECTED, - // 401: the session holds no usable connection, which is not an error. - bucketsError: - res.status === 401 ? null : res.status === 403 ? 'access_denied' : 'unreachable', + bucketsError: bucketsErrorFor(res.status), connections, hydrating: false as const, hasActiveConnection: false as const @@ -89,7 +95,7 @@ export const load: LayoutLoad = async ({ fetch, data }) => { } catch { return { ...DISCONNECTED, - bucketsError: 'unreachable', + bucketsError: 'unreachable' as const, connections, hydrating: false as const, hasActiveConnection: false as const diff --git a/src/routes/(app)/storage/error.svelte.spec.ts b/src/routes/(app)/storage/error.svelte.spec.ts index db5e9838..098c43e8 100644 --- a/src/routes/(app)/storage/error.svelte.spec.ts +++ b/src/routes/(app)/storage/error.svelte.spec.ts @@ -89,6 +89,20 @@ describe('/(app)/storage/+error.svelte', () => { .not.toBeInTheDocument(); }); + it('offers storage-specific ways out instead of the dashboard link', async () => { + setPage({ pathname: '/storage', status: 502 }); + render(ErrorPage); + + await expect.element(page.getByRole('heading', { name: /Storage error/ })).toBeInTheDocument(); + await expect + .element(page.getByRole('link', { name: 'Back to storage' })) + .toHaveAttribute('href', '/storage'); + await expect.element(page.getByRole('button', { name: 'Go back' })).toBeInTheDocument(); + await expect + .element(page.getByRole('link', { name: 'Go to dashboard' })) + .not.toBeInTheDocument(); + }); + it('does not throw on malformed percent-encoding in the URL', async () => { setPage({ pathname: '/storage/browse/conn-1/bad%E0bucket', params: {}, status: 404 }); render(ErrorPage); diff --git a/src/routes/+error.svelte b/src/routes/+error.svelte index f45703d4..efa7d4e0 100644 --- a/src/routes/+error.svelte +++ b/src/routes/+error.svelte @@ -1,9 +1,24 @@ - +
- + {#if isAuthPage} + + {#snippet actions()} + + {/snippet} + + {:else} + + {/if}
diff --git a/src/routes/error.svelte.spec.ts b/src/routes/error.svelte.spec.ts new file mode 100644 index 00000000..bcf69c58 --- /dev/null +++ b/src/routes/error.svelte.spec.ts @@ -0,0 +1,32 @@ +import { page } from 'vitest/browser'; +import { describe, expect, it, vi } from 'vitest'; +import { render } from 'vitest-browser-svelte'; + +const mockPage = vi.hoisted(() => ({ + url: new URL('http://localhost/'), + status: 500, + error: { message: 'An unexpected error occurred' } as App.Error | null +})); + +vi.mock('$app/state', () => ({ page: mockPage })); + +import ErrorPage from './+error.svelte'; + +describe('/+error.svelte', () => { + it('does not link a failing sign-in page to the dashboard, which requires sign-in', async () => { + mockPage.url = new URL('http://localhost/auth/login'); + render(ErrorPage); + + await expect.element(page.getByRole('button', { name: 'Try again' })).toBeInTheDocument(); + await expect + .element(page.getByRole('link', { name: 'Go to dashboard' })) + .not.toBeInTheDocument(); + }); + + it('links to the dashboard elsewhere', async () => { + mockPage.url = new URL('http://localhost/'); + render(ErrorPage); + + await expect.element(page.getByRole('link', { name: 'Go to dashboard' })).toBeInTheDocument(); + }); +}); From 68675ba18b1b5f1598bac3523584c6575b7561f0 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Wed, 28 Oct 2026 13:19:30 +0100 Subject: [PATCH 5/5] test: expect the merged message for an invalid connection id (#359) --- e2e/storage/connections-management.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/storage/connections-management.spec.ts b/e2e/storage/connections-management.spec.ts index 66ab4479..e05a9686 100644 --- a/e2e/storage/connections-management.spec.ts +++ b/e2e/storage/connections-management.spec.ts @@ -343,7 +343,7 @@ test.describe('Storage — Connections management', () => { form.submit(); }); - await expect(page.getByRole('alert')).toHaveText('The selected connection is invalid.'); + await expect(page.getByRole('alert')).toHaveText('Connection not found.'); }); });