From 4990e0d59c69c6713fdb8b01277ef2afbb12e34b Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Thu, 22 Oct 2026 14:40:41 +0200 Subject: [PATCH 1/3] fix(storage): apply superforms fixes to the storage connection forms The storage connect and connection edit forms came in with the S3 file browser merge and missed the fixes from #346. - Show network and server failures in the form instead of the error page (onError, caught save/session/update errors) - Translate schema validation messages; an untouched host now reports "Host is required" instead of zod's default - Treat a cleared port field (null) as the default port - Return translated errors from the use/delete actions and show them - Replace the hand-rolled unsaved-changes guard on the edit page with superforms' taintedMessage - Always test the connection when editing, with the stored credentials if none are entered - Share the connection test error mapping and detect network errors by error code (AggregateError for localhost has an empty message) - Log through locals.logger in the edit action --- e2e/storage/connection.spec.ts | 12 + e2e/storage/connections-management.spec.ts | 52 ++ messages/de.json | 8 + messages/en.json | 8 + .../storage/landing/StorageConnectForm.svelte | 8 +- .../landing/StorageConnectForm.svelte.spec.ts | 14 +- .../storage/connection-test-error.test.ts | 66 +++ .../server/storage/connection-test-error.ts | 39 ++ src/lib/storage/schemas.spec.ts | 31 ++ src/lib/storage/schemas.ts | 50 +- .../settings/connections/+page.server.ts | 30 +- .../(app)/settings/connections/+page.svelte | 5 + .../connections/[id]/edit/+page.server.ts | 157 +++--- .../connections/[id]/edit/+page.svelte | 456 ++++++++---------- src/routes/(app)/storage/+page.server.ts | 116 ++--- src/routes/(app)/storage/page.server.test.ts | 50 +- 16 files changed, 682 insertions(+), 420 deletions(-) create mode 100644 src/lib/server/storage/connection-test-error.test.ts create mode 100644 src/lib/server/storage/connection-test-error.ts diff --git a/e2e/storage/connection.spec.ts b/e2e/storage/connection.spec.ts index 2ce6c8f7..d0130613 100644 --- a/e2e/storage/connection.spec.ts +++ b/e2e/storage/connection.spec.ts @@ -46,6 +46,18 @@ test.describe('Storage S3 — Connection', () => { await expect(page.getByText('Access denied — check your credentials.')).toBeVisible(); }); + test('a cleared port field means the default port, not a validation error', async ({ page }) => { + await openConnectForm(page); + const port = page.getByLabel('Port'); + await port.fill('9000'); + await port.fill(''); + await page.getByRole('button', { name: 'Connect', exact: true }).click(); + + // The empty host still fails, so validation has run. + await expect(page.getByText('Host is required')).toBeVisible(); + await expect(page.getByText(/Port must be|Too small/)).toHaveCount(0); + }); + 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..18853bab 100644 --- a/e2e/storage/connections-management.spec.ts +++ b/e2e/storage/connections-management.spec.ts @@ -3,6 +3,8 @@ import { hasGarageCredentials, requireGarageCredentials } from '../support/garag import { connectToStorage, openConnectForm } from './helpers.js'; import { waitForHydration } from '../support/helpers.js'; +const PORT_1_ERROR = 'Could not reach the storage backend — check the host and port.'; + /** Open the Options context menu for the first connection row and click Edit. */ async function openFirstConnectionEditPage(page: Page) { await page.locator('table tbody tr').first().waitFor(); @@ -149,6 +151,22 @@ test.describe('Storage — Connections management', () => { await expect(page.getByText('Access denied — check your credentials.')).toBeVisible(); }); + test('edit page tests the connection when only the endpoint changes', async ({ page }) => { + const credentials = requireGarageCredentials(); + await connectToStorage(page, credentials); + await page.goto('/settings/connections'); + await waitForHydration(page); + + await openFirstConnectionEditPage(page); + + // Credentials stay blank (keep the stored ones); nothing listens on port 1. + await page.getByLabel('Port').fill('1'); + await page.getByRole('button', { name: 'Save changes' }).click(); + + await expect(page.getByText(PORT_1_ERROR)).toBeVisible(); + await expect(page).toHaveURL(/\/edit$/); + }); + test('delete button on management page removes the connection after confirmation', async ({ page }) => { @@ -231,6 +249,40 @@ test.describe('Storage — Connections management', () => { await expect(page.getByRole('heading', { name: 'Edit connection' })).toBeVisible(); }); + test('edit page without changes leaves without asking', async ({ page }) => { + const credentials = requireGarageCredentials(); + await connectToStorage(page, credentials); + await page.goto('/settings/connections'); + await waitForHydration(page); + + await openFirstConnectionEditPage(page); + await page.getByRole('link', { name: '← Manage connections' }).click(); + + await expect(page).toHaveURL('/settings/connections'); + await expect(page.getByRole('dialog')).toHaveCount(0); + }); + + test('unsaved-changes modal: Escape stays and the guard asks again', async ({ page }) => { + const credentials = requireGarageCredentials(); + await connectToStorage(page, credentials); + await page.goto('/settings/connections'); + await waitForHydration(page); + + await openFirstConnectionEditPage(page); + + await page.getByLabel('Connection name').fill('Changed name'); + const backLink = page.getByRole('link', { name: '← Manage connections' }); + await backLink.click(); + await expect(page.getByRole('dialog')).toBeVisible(); + + await page.keyboard.press('Escape'); + await expect(page.getByRole('dialog')).toBeHidden(); + await expect(page).toHaveURL(/\/edit$/); + + await backLink.click(); + await expect(page.getByRole('heading', { name: 'Unsaved changes' })).toBeVisible(); + }); + test('unsaved-changes modal: Leave navigates away', async ({ page }) => { const credentials = requireGarageCredentials(); await connectToStorage(page, credentials); diff --git a/messages/de.json b/messages/de.json index b7ce2634..d15bc568 100644 --- a/messages/de.json +++ b/messages/de.json @@ -166,6 +166,13 @@ "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_save_failed": "Die Verbindung konnte nicht gespeichert werden. Bitte erneut versuchen.", + "storage_connect_host_required": "Host ist erforderlich", + "storage_connect_host_invalid": "Host muss ein gültiger Hostname oder eine gültige IP-Adresse sein", + "storage_connect_port_invalid": "Port muss eine ganze Zahl zwischen 1 und 65535 sein", + "storage_connect_region_required": "Region ist erforderlich", + "storage_connect_access_key_required": "Zugriffsschlüssel ist erforderlich, wenn ein geheimer Schlüssel angegeben ist", + "storage_connect_secret_key_required": "Geheimer Schlüssel ist erforderlich, wenn ein Zugriffsschlüssel angegeben ist", "storage_connect_saved": "Gespeicherte Verbindungen", "storage_connect_no_saved": "Noch keine gespeicherten Verbindungen", "storage_connect_forget": "Löschen", @@ -202,6 +209,7 @@ "storage_connections_delete_confirm": "\"{label}\" aus den gespeicherten Verbindungen entfernen? Dies kann nicht rückgängig gemacht werden.", "storage_connections_delete_cancel": "Abbrechen", "storage_connections_delete_confirm_button": "Löschen", + "storage_connections_delete_failed": "Die Verbindung konnte nicht gelöscht werden. Bitte erneut versuchen.", "storage_connection_edit_title": "Verbindung bearbeiten", "storage_connection_edit_name": "Verbindungsname", "storage_connection_edit_name_placeholder": "z. B. Produktions-Garage", diff --git a/messages/en.json b/messages/en.json index d64250a1..7b677e13 100644 --- a/messages/en.json +++ b/messages/en.json @@ -166,6 +166,13 @@ "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_save_failed": "Could not save the connection. Please try again.", + "storage_connect_host_required": "Host is required", + "storage_connect_host_invalid": "Host must be a valid hostname or IP address", + "storage_connect_port_invalid": "Port must be a whole number between 1 and 65535", + "storage_connect_region_required": "Region is required", + "storage_connect_access_key_required": "Access key is required when a secret key is provided", + "storage_connect_secret_key_required": "Secret key is required when an access key is provided", "storage_connect_saved": "Saved connections", "storage_connect_no_saved": "No saved connections yet", "storage_connect_forget": "Delete", @@ -202,6 +209,7 @@ "storage_connections_delete_confirm": "Remove “{label}” from saved connections? This cannot be undone.", "storage_connections_delete_cancel": "Cancel", "storage_connections_delete_confirm_button": "Delete", + "storage_connections_delete_failed": "Could not delete the connection. Please try again.", "storage_connection_edit_title": "Edit connection", "storage_connection_edit_name": "Connection name", "storage_connection_edit_name_placeholder": "e.g. Production Garage", diff --git a/src/lib/components/storage/landing/StorageConnectForm.svelte b/src/lib/components/storage/landing/StorageConnectForm.svelte index c15ff6b3..1510eda5 100644 --- a/src/lib/components/storage/landing/StorageConnectForm.svelte +++ b/src/lib/components/storage/landing/StorageConnectForm.svelte @@ -27,9 +27,9 @@ { dataType: 'json', validators: zod(StorageConnectionSchema), - onResult: () => { - // Server saved the connection; the redirect to /storage triggers - // the layout load to auto-connect via activeStorageConnectionId. + // Network failures or unexpected server errors: show them in the form instead of the error page. + onError() { + $message = { type: 'error', message: m.storage_connect_save_failed() }; } } ); @@ -57,7 +57,7 @@ {#if connectError} {/if} diff --git a/src/lib/components/storage/landing/StorageConnectForm.svelte.spec.ts b/src/lib/components/storage/landing/StorageConnectForm.svelte.spec.ts index 95909388..4b998b9c 100644 --- a/src/lib/components/storage/landing/StorageConnectForm.svelte.spec.ts +++ b/src/lib/components/storage/landing/StorageConnectForm.svelte.spec.ts @@ -47,7 +47,13 @@ vi.mock('$lib/paraglide/messages.js', () => ({ `Forget connection to ${endpoint}`, storage_connect_forget_confirm: ({ endpoint }: { endpoint: string }) => `Remove ${endpoint} from saved connections?`, - storage_connect_error_unreachable: () => 'Could not connect', + storage_connect_save_failed: () => 'Could not save the connection', + storage_connect_host_required: () => 'Host is required', + storage_connect_host_invalid: () => 'Host is invalid', + storage_connect_port_invalid: () => 'Port is invalid', + storage_connect_region_required: () => 'Region is required', + storage_connect_access_key_required: () => 'Access key is required', + storage_connect_secret_key_required: () => 'Secret key is required', storage_connect_manage: () => 'Manage connections', storage_connect_edit: () => 'Edit', storage_more_options: () => 'More options', @@ -412,4 +418,10 @@ describe('StorageConnectForm', () => { await expect.element(page.getByText('Secret key required')).toBeInTheDocument(); await expect.element(page.getByText('Validation failed')).toBeInTheDocument(); }); + + it('should show the error returned by the use action', async () => { + renderForm({}, [], 'Connection not found.'); + + await expect.element(page.getByRole('alert')).toHaveTextContent('Connection not found.'); + }); }); diff --git a/src/lib/server/storage/connection-test-error.test.ts b/src/lib/server/storage/connection-test-error.test.ts new file mode 100644 index 00000000..6ab24ad2 --- /dev/null +++ b/src/lib/server/storage/connection-test-error.test.ts @@ -0,0 +1,66 @@ +import { describe, it, expect } from 'vitest'; +import { error } from '@sveltejs/kit'; +import { connectionTestErrorMessage } from './connection-test-error.js'; + +function httpError(status: number): unknown { + try { + error(status, 'test'); + } catch (err) { + return err; + } +} + +describe('connectionTestErrorMessage', () => { + it('maps HTTP errors by status', () => { + expect(connectionTestErrorMessage(httpError(403))).toBe( + 'Access denied — check your credentials.' + ); + expect(connectionTestErrorMessage(httpError(404))).toBe( + 'Could not find the storage backend — check the endpoint.' + ); + expect(connectionTestErrorMessage(httpError(502))).toBe( + 'The storage backend returned an error — check the endpoint and credentials.' + ); + expect(connectionTestErrorMessage(httpError(500))).toBe( + 'Could not connect — check the endpoint and credentials.' + ); + }); + + it('maps network errors', () => { + expect(connectionTestErrorMessage(new Error('connect ECONNREFUSED 127.0.0.1:9000'))).toBe( + 'Could not reach the storage backend — check the host and port.' + ); + }); + + it('maps network errors by code when the message is empty', () => { + // Connecting to localhost tries IPv6 and IPv4; Node reports both in an AggregateError. + const refused = (address: string) => + Object.assign(new Error(`connect ECONNREFUSED ${address}`), { code: 'ECONNREFUSED' }); + const aggregate = Object.assign( + new AggregateError([refused('::1:1'), refused('127.0.0.1:1')], ''), + { code: 'ECONNREFUSED' } + ); + expect(connectionTestErrorMessage(aggregate)).toBe( + 'Could not reach the storage backend — check the host and port.' + ); + + const inner = new AggregateError([refused('::1:1')], ''); + expect(connectionTestErrorMessage(inner)).toBe( + 'Could not reach the storage backend — check the host and port.' + ); + + const notFound = Object.assign(new Error(''), { code: 'ENOTFOUND' }); + expect(connectionTestErrorMessage(notFound)).toBe( + 'Could not reach the storage backend — check the host and port.' + ); + }); + + it('falls back to the generic message', () => { + expect(connectionTestErrorMessage(new Error('boom'))).toBe( + 'Could not connect — check the endpoint and credentials.' + ); + expect(connectionTestErrorMessage('not an error')).toBe( + 'Could not connect — check the endpoint and credentials.' + ); + }); +}); diff --git a/src/lib/server/storage/connection-test-error.ts b/src/lib/server/storage/connection-test-error.ts new file mode 100644 index 00000000..ce7a3b0c --- /dev/null +++ b/src/lib/server/storage/connection-test-error.ts @@ -0,0 +1,39 @@ +import { isHttpError } from '@sveltejs/kit'; +import * as m from '$lib/paraglide/messages.js'; + +const NETWORK_ERROR_CODES = new Set([ + 'ECONNREFUSED', + 'ECONNRESET', + 'ENOTFOUND', + 'EAI_AGAIN', + 'ETIMEDOUT', + 'EHOSTUNREACH', + 'ENETUNREACH' +]); + +/** + * Whether a connection failure comes from the network rather than from S3. Checks the Node error + * code: connecting to `localhost` tries IPv6 and IPv4 and fails with an AggregateError whose + * message is empty, but whose code (and each inner error's code) is set. + */ +function isNetworkError(err: Error): boolean { + const errors = [err, ...(err instanceof AggregateError ? err.errors : [])]; + if (errors.some((e) => NETWORK_ERROR_CODES.has((e as { code?: unknown })?.code as string))) { + return true; + } + return /econnrefused|enotfound|eai_again|etimedout|network/.test(err.message.toLowerCase()); +} + +/** User-facing message for a failed connection test (listing the buckets of a new connection). */ +export function connectionTestErrorMessage(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 && isNetworkError(err)) { + return m.storage_connect_error_network(); + } + return m.storage_connect_error(); +} diff --git a/src/lib/storage/schemas.spec.ts b/src/lib/storage/schemas.spec.ts index 5cd09265..5a493b34 100644 --- a/src/lib/storage/schemas.spec.ts +++ b/src/lib/storage/schemas.spec.ts @@ -87,6 +87,11 @@ describe('StorageConnectionSchema', () => { it('rejects port out of range', () => { const result = StorageConnectionSchema.safeParse({ host: 's3.example.com', port: 99999 }); expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].message).toBe( + 'Port must be a whole number between 1 and 65535' + ); + } }); it('accepts empty string port as undefined', () => { @@ -97,6 +102,32 @@ describe('StorageConnectionSchema', () => { } }); + it('rejects a non-numeric port with the port message', () => { + const result = StorageConnectionSchema.safeParse({ host: 's3.example.com', port: 'abc' }); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].message).toBe( + 'Port must be a whole number between 1 and 65535' + ); + } + }); + + it('reports a missing host as required', () => { + const result = StorageConnectionSchema.safeParse({}); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].message).toBe('Host is required'); + } + }); + + it('accepts a cleared number input (null) as undefined', () => { + const result = StorageConnectionSchema.safeParse({ host: 's3.example.com', port: null }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.port).toBeUndefined(); + } + }); + it('accepts tls verification', () => { const result = StorageConnectionSchema.safeParse({ host: 's3.example.com', diff --git a/src/lib/storage/schemas.ts b/src/lib/storage/schemas.ts index c57df1cd..3ecce3f8 100644 --- a/src/lib/storage/schemas.ts +++ b/src/lib/storage/schemas.ts @@ -1,4 +1,5 @@ import { z } from 'zod'; +import * as m from '$lib/paraglide/messages.js'; import { MAX_TRANSFER_ITEMS } from './transfer-types.js'; function isValidHostname(host: string): boolean { @@ -45,33 +46,46 @@ export function hasEmptyPathSegment(value: string): boolean { /** S3 limits object keys to 1024 bytes of UTF-8. */ const MAX_OBJECT_KEY_BYTES = 1024; +const portError = { error: () => m.storage_connect_port_invalid() }; + const baseStorageConnectionObject = z.object({ id: z.string().uuid().optional(), name: z.string().optional(), type: z.enum(['s3', 'hdfs']).default('s3'), + // An untouched host field is undefined, so the type check needs the "required" message too. host: z - .string() + .string({ error: () => m.storage_connect_host_required() }) .trim() - .min(1, 'Host is required') - .refine((value) => { - if (!value.includes('://')) return true; - return z.url().safeParse(value).success; - }, 'Host must be a valid hostname or IP address') + .min(1, { error: () => m.storage_connect_host_required() }) + .refine( + (value) => { + if (!value.includes('://')) return true; + return z.url().safeParse(value).success; + }, + { error: () => m.storage_connect_host_invalid() } + ) .transform((v) => { if (v.includes('://')) return new URL(v).hostname; return isIpv6(v) ? `[${v}]` : v; }) - .refine(isValidHost, 'Host must be a valid hostname or IP address'), - port: z.coerce - .number() - .int() - .min(1) - .max(65535) - .optional() - .or(z.literal('').transform(() => undefined)), + .refine(isValidHost, { error: () => m.storage_connect_host_invalid() }), + // A cleared number input is bound as `null` and an empty form field arrives as `''`; both mean + // "default port". Without the explicit branches, coercion would turn them into 0. + port: z + .union( + [ + z.coerce.number(portError).int(portError).min(1, portError).max(65535, portError), + z.literal('').transform(() => undefined), + z.null().transform(() => undefined) + ], + portError + ) + .optional(), tls: z.object({ verification: z.enum(['Full', 'None']) }).optional(), accessStyle: z.enum(['Path', 'VirtualHosted']).default('VirtualHosted'), - region: z.object({ name: z.string().min(1) }).default({ name: 'us-east-1' }), + region: z + .object({ name: z.string().min(1, { error: () => m.storage_connect_region_required() }) }) + .default({ name: 'us-east-1' }), credentials: z .object({ accessKey: z.string(), @@ -87,14 +101,14 @@ export const StorageConnectionSchema = baseStorageConnectionObject.superRefine(( ctx.addIssue({ code: 'custom', path: ['credentials', 'secretKey'], - message: 'Secret key is required when access key is provided' + message: m.storage_connect_secret_key_required() }); } if (!hasKey && hasSecret) { ctx.addIssue({ code: 'custom', path: ['credentials', 'accessKey'], - message: 'Access key is required when secret key is provided' + message: m.storage_connect_access_key_required() }); } }); @@ -109,7 +123,7 @@ export const EditStorageConnectionSchema = baseStorageConnectionObject.superRefi ctx.addIssue({ code: 'custom', path: ['credentials', 'accessKey'], - message: 'Access key is required when secret key is provided' + message: m.storage_connect_access_key_required() }); } }); diff --git a/src/routes/(app)/settings/connections/+page.server.ts b/src/routes/(app)/settings/connections/+page.server.ts index 638de09f..868c1596 100644 --- a/src/routes/(app)/settings/connections/+page.server.ts +++ b/src/routes/(app)/settings/connections/+page.server.ts @@ -4,6 +4,7 @@ import { superValidate } from 'sveltekit-superforms'; import { zod4 as zod } from 'sveltekit-superforms/adapters'; import { desc, eq } from 'drizzle-orm'; import { ConnectionIdSchema, StoredStorageConnectionSchema } from '$lib/storage/schemas.js'; +import * as m from '$lib/paraglide/messages.js'; import { deleteConnection } from '$lib/server/storage/connections-db.js'; import { auth } from '$lib/server/auth.js'; import { db } from '$lib/server/db.js'; @@ -54,23 +55,32 @@ export const actions: Actions = { const form = await superValidate(request, zod(ConnectionIdSchema)); if (!form.valid) { - return fail(400, { error: 'Invalid connection ID' }); + log.debug({ errors: form.errors }, 'invalid connection id for delete'); + return fail(400, { error: m.storage_connection_edit_not_found() }); } const { connectionId } = form.data; const userId = locals.user!.id; const activeId = locals.session?.activeStorageConnectionId ?? null; - if (connectionId === activeId) { - await auth.api.updateSession({ - headers: request.headers, - body: { activeStorageConnectionId: null } - }); - log.info({ connectionId }, 'active storage connection deleted, session cleared'); - } + try { + if (connectionId === activeId) { + await auth.api.updateSession({ + headers: request.headers, + body: { activeStorageConnectionId: null } + }); + log.info( + { connection_id: connectionId }, + 'active storage connection deleted, session cleared' + ); + } - await deleteConnection(userId, connectionId); - log.info({ connectionId }, 'storage connection deleted from management page'); + await deleteConnection(userId, connectionId); + log.info({ connection_id: connectionId }, 'storage connection deleted from management page'); + } catch (err) { + log.error({ err, connection_id: connectionId }, 'failed to delete storage connection'); + return fail(500, { error: m.storage_connections_delete_failed() }); + } throw redirect(303, '/settings/connections'); } diff --git a/src/routes/(app)/settings/connections/+page.svelte b/src/routes/(app)/settings/connections/+page.svelte index 2845e9d9..533aec76 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 deleteError = $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 deleteError} + + {/if} + {#if connections.length === 0}

{m.storage_connections_empty()}

{:else} diff --git a/src/routes/(app)/settings/connections/[id]/edit/+page.server.ts b/src/routes/(app)/settings/connections/[id]/edit/+page.server.ts index ab79b71a..49af8b8e 100644 --- a/src/routes/(app)/settings/connections/[id]/edit/+page.server.ts +++ b/src/routes/(app)/settings/connections/[id]/edit/+page.server.ts @@ -1,4 +1,4 @@ -import { error, fail, redirect, isHttpError } from '@sveltejs/kit'; +import { error, fail, redirect } from '@sveltejs/kit'; import { superValidate, message, setError } from 'sveltekit-superforms'; import type { Infer, InferIn } from 'sveltekit-superforms'; import { zod4 as zod } from 'sveltekit-superforms/adapters'; @@ -11,20 +11,18 @@ import { } from '$lib/storage/schemas.js'; import type { z } from 'zod'; import { getConnectionProvider } from '$lib/server/storage/utils.js'; +import { connectionTestErrorMessage } from '$lib/server/storage/connection-test-error.js'; import type { S3ConnectionConfig } from '$lib/server/storage/types.js'; import { db } from '$lib/server/db.js'; import { userStorageConnections } from '$lib/server/schema.js'; import { decrypt, encrypt, fingerprint } from '$lib/server/storage/encryption.js'; import { isConnectionNameConflict } from '$lib/server/storage/connections-db.js'; import { storageEncryptionKey } from '$lib/server/storage/encryption-key.js'; -import { logger } from '$lib/server/logging'; import * as m from '$lib/paraglide/messages.js'; type EditData = Infer; type EditInput = InferIn; -const log = logger.child({ module: 'connection-edit' }); - export const load: PageServerLoad = async ({ locals, params }) => { const userId = locals.user!.id; @@ -80,6 +78,7 @@ export const load: PageServerLoad = async ({ locals, params }) => { export const actions: Actions = { update: async ({ request, locals, params }) => { + const log = locals.logger; const form = await superValidate( request, zod(EditStorageConnectionSchema) @@ -102,8 +101,52 @@ export const actions: Actions = { ); } - const resolvedCredentials = - credentials.accessKey && credentials.secretKey ? credentials : undefined; + let existing: z.infer; + try { + const rows = await db + .select({ encryptedPayload: userStorageConnections.encryptedPayload }) + .from(userStorageConnections) + .where( + and( + eq(userStorageConnections.id, connectionId), + eq(userStorageConnections.userId, userId) + ) + ) + .limit(1); + if (rows.length === 0) { + log.debug({ connection_id: connectionId }, 'storage connection to update not found'); + return message( + form, + { type: 'error', message: m.storage_connection_edit_not_found() }, + { status: 404 } + ); + } + existing = StoredStorageConnectionSchema.parse( + JSON.parse(decrypt(rows[0].encryptedPayload, storageEncryptionKey())) + ); + } catch (err) { + log.error( + { err, connection_id: connectionId }, + 'failed to load storage connection to update' + ); + return message( + form, + { type: 'error', message: m.storage_connect_save_failed() }, + { status: 500 } + ); + } + + // Blank credentials keep the stored ones; a new access key needs its secret key. + const newCredentials = credentials.accessKey && credentials.secretKey ? credentials : undefined; + if ( + !newCredentials && + credentials.accessKey && + credentials.accessKey !== existing.credentials?.accessKey + ) { + log.debug({ connection_id: connectionId }, 'access key changed without a secret key'); + return setError(form, 'credentials.secretKey', m.storage_connection_edit_secret_required()); + } + const resolvedCredentials = newCredentials ?? existing.credentials; const config: S3ConnectionConfig = { type: 's3', @@ -112,86 +155,29 @@ export const actions: Actions = { tls, accessStyle, region, - credentials: resolvedCredentials + credentials: resolvedCredentials?.accessKey ? resolvedCredentials : undefined }; - if (resolvedCredentials) { - try { - await getConnectionProvider(config).listContainers(); - log.info({ storage_type: type }, 'storage connection edit verified'); - } catch (err) { - log.warn({ err }, 'storage connection edit 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 }); - } - } - - // If no new credentials were provided, preserve the existing encrypted ones. - let payload: z.infer; - if (resolvedCredentials) { - payload = { host, port, tls, accessStyle, region, credentials: resolvedCredentials }; - } else { - try { - const rows = await db - .select({ encryptedPayload: userStorageConnections.encryptedPayload }) - .from(userStorageConnections) - .where( - and( - eq(userStorageConnections.id, connectionId), - eq(userStorageConnections.userId, userId) - ) - ) - .limit(1); - if (rows.length === 0) { - return message( - form, - { type: 'error', message: m.storage_connect_error_not_found() }, - { status: 404 } - ); - } - const existing = StoredStorageConnectionSchema.parse( - JSON.parse(decrypt(rows[0].encryptedPayload, storageEncryptionKey())) - ); - if (credentials.accessKey && credentials.accessKey !== existing.credentials?.accessKey) { - log.debug({ connection_id: connectionId }, 'access key changed without a secret key'); - return setError( - form, - 'credentials.secretKey', - m.storage_connection_edit_secret_required() - ); - } - payload = { host, port, tls, accessStyle, region, credentials: existing.credentials }; - } catch { - return message( - form, - { type: 'error', message: m.storage_connect_error() }, - { status: 500 } - ); - } + try { + await getConnectionProvider(config).listContainers(); + log.info({ connection_id: connectionId }, 'storage connection edit verified'); + } catch (err) { + log.warn({ err, connection_id: connectionId }, 'storage connection edit test failed'); + return message( + form, + { type: 'error', message: connectionTestErrorMessage(err) }, + { status: 400 } + ); } + const payload: z.infer = { + host, + port, + tls, + accessStyle, + region, + credentials: resolvedCredentials + }; const encryptedPayload = encrypt(JSON.stringify(payload), storageEncryptionKey()); const hash = fingerprint( { @@ -223,7 +209,12 @@ export const actions: Actions = { log.debug({ connection_id: connectionId }, 'storage connection name already in use'); return setError(form, 'name', m.storage_connection_edit_name_taken()); } - throw err; + log.error({ err, connection_id: connectionId }, 'failed to update storage connection'); + return message( + form, + { type: 'error', message: m.storage_connect_save_failed() }, + { status: 500 } + ); } log.info({ connection_id: connectionId }, 'storage connection updated'); diff --git a/src/routes/(app)/settings/connections/[id]/edit/+page.svelte b/src/routes/(app)/settings/connections/[id]/edit/+page.svelte index dc82a773..e590335e 100644 --- a/src/routes/(app)/settings/connections/[id]/edit/+page.svelte +++ b/src/routes/(app)/settings/connections/[id]/edit/+page.svelte @@ -1,94 +1,62 @@ - -

{m.storage_connection_edit_unsaved_title()}

{m.storage_connection_edit_unsaved_body()}

- -
@@ -122,210 +88,204 @@

{m.storage_connection_edit_title()}

-{#if !loaded} -
- +{#if isActiveConnection} +
+
-{:else} - {#if isActiveConnection} -
-
- {/if} +{/if} -
- + + - -
- - - {#if $errors?.name} -

{$errors.name}

- {/if} -

{m.storage_connection_edit_name_hint()}

-
+ +
+ + + {#if $errors?.name} +

{$errors.name}

+ {/if} +

{m.storage_connection_edit_name_hint()}

+
- + - -
- - - {#if $errors?.host} -

{$errors.host}

- {/if} -
+ +
+ + + {#if $errors?.host} +

{$errors.host}

+ {/if} +
+ + +
+ + +

{m.storage_connect_port_hint()}

+ {#if $errors?.port} +

{$errors.port}

+ {/if} +
- + +
-
+ { + $form.tls = e.currentTarget.checked ? { verification: 'Full' } : undefined; + }} + /> +
- -
+ {#if $form.tls} +
-
{ - $form.tls = e.currentTarget.checked ? { verification: 'Full' } : undefined; + $form.tls = { verification: e.currentTarget.checked ? 'Full' : 'None' }; }} />
+ {/if} - {#if $form.tls} -
-
- -

- {m.storage_connect_tls_verification_hint()} -

-
- { - $form.tls = { verification: e.currentTarget.checked ? 'Full' : 'None' }; - }} - /> -
- {/if} + +
+ + +
- -
- - -
+ +
+ + + {#if $errors?.region?.name} +

{$errors.region.name}

+ {/if} +
- -
- - - {#if $errors?.region?.name} -

{$errors.region.name}

- {/if} -
+ +
+ + + {#if $errors?.credentials?.accessKey} +

{$errors.credentials.accessKey}

+ {/if} +
- -
- - - {#if $errors?.credentials?.accessKey} -

{$errors.credentials.accessKey}

- {/if} -
+ +
+ + +

+ {m.storage_connection_edit_credentials_hint()} +

+ {#if $errors?.credentials?.secretKey} +

{$errors.credentials.secretKey}

+ {/if} +
- -
- - -

- {m.storage_connection_edit_credentials_hint()} -

- {#if $errors?.credentials?.secretKey} -

{$errors.credentials.secretKey}

- {/if} -
+ {#if $message?.type === 'error'} +

{$message.message}

+ {/if} - {#if $message?.type === 'error'} -

{$message.message}

+ + - - - -
- - ← {m.storage_connections_title()} - -
-{/if} +
+ + ← {m.storage_connections_title()} + +
diff --git a/src/routes/(app)/storage/+page.server.ts b/src/routes/(app)/storage/+page.server.ts index 73d2e4eb..3afa3ccf 100644 --- a/src/routes/(app)/storage/+page.server.ts +++ b/src/routes/(app)/storage/+page.server.ts @@ -1,4 +1,4 @@ -import { fail, redirect, isHttpError } from '@sveltejs/kit'; +import { fail, redirect } from '@sveltejs/kit'; import { superValidate, message } from 'sveltekit-superforms'; import { zod4 as zod } from 'sveltekit-superforms/adapters'; import { desc, eq } from 'drizzle-orm'; @@ -11,6 +11,7 @@ import { type StorageConnectionMessage } from '$lib/storage/schemas.js'; import { getConnectionProvider } from '$lib/server/storage/utils.js'; +import { connectionTestErrorMessage } from '$lib/server/storage/connection-test-error.js'; import { saveConnection, getConnectionForUser, @@ -106,37 +107,27 @@ 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: connectionTestErrorMessage(err) }, + { status: 400 } + ); } - const id = await saveConnection(locals.user!.id, config); - await auth.api.updateSession({ - headers: request.headers, - body: { activeStorageConnectionId: id } - }); + try { + const id = await saveConnection(locals.user!.id, config); + await auth.api.updateSession({ + headers: request.headers, + body: { activeStorageConnectionId: id } + }); + } catch (err) { + log.error({ err }, 'failed to save storage connection'); + return message( + form, + { type: 'error', message: m.storage_connect_save_failed() }, + { status: 500 } + ); + } throw redirect(303, '/storage'); }, @@ -155,30 +146,37 @@ export const actions: Actions = { const form = await superValidate(request, zod(ConnectionIdSchema)); if (!form.valid) { - return fail(400, { error: 'Invalid connection ID' }); + log.debug({ errors: form.errors }, 'invalid connection id for use'); + return fail(400, { error: m.storage_connection_edit_not_found() }); } const { connectionId } = form.data; const userId = locals.user!.id; - const config = await getConnectionForUser(userId, connectionId); - if (!config) { - return fail(400, { error: 'Connection not found' }); - } - try { - await getConnectionProvider(config).listContainers(); - log.info({ connectionId }, 'user switched storage connection'); + const config = await getConnectionForUser(userId, connectionId); + if (!config) { + log.debug({ connection_id: connectionId }, 'storage connection to use not found'); + return fail(400, { error: m.storage_connection_edit_not_found() }); + } + + try { + await getConnectionProvider(config).listContainers(); + } catch (err) { + log.warn({ err, connection_id: connectionId }, 'storage connection test failed on use'); + return fail(400, { error: m.storage_connect_error_unreachable() }); + } + + await auth.api.updateSession({ + headers: request.headers, + body: { activeStorageConnectionId: connectionId } + }); + log.info({ connection_id: 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.' }); + log.error({ err, connection_id: connectionId }, 'failed to switch storage connection'); + return fail(500, { error: m.storage_connect_error() }); } - await auth.api.updateSession({ - headers: request.headers, - body: { activeStorageConnectionId: connectionId } - }); - throw redirect(303, '/storage'); }, @@ -187,22 +185,32 @@ export const actions: Actions = { const form = await superValidate(request, zod(ConnectionIdSchema)); if (!form.valid) { - return fail(400, { error: 'Invalid connection ID' }); + log.debug({ errors: form.errors }, 'invalid connection id for delete'); + return fail(400, { error: m.storage_connection_edit_not_found() }); } const { connectionId } = form.data; const userId = locals.user!.id; const activeId = locals.session?.activeStorageConnectionId ?? null; - if (connectionId === activeId) { - await auth.api.updateSession({ - headers: request.headers, - body: { activeStorageConnectionId: null } - }); - log.info({ connectionId }, 'active storage connection deleted, session cleared'); - } + try { + if (connectionId === activeId) { + await auth.api.updateSession({ + headers: request.headers, + body: { activeStorageConnectionId: null } + }); + log.info( + { connection_id: connectionId }, + 'active storage connection deleted, session cleared' + ); + } - await deleteConnection(userId, connectionId); + await deleteConnection(userId, connectionId); + log.info({ connection_id: connectionId }, 'storage connection deleted'); + } catch (err) { + log.error({ err, connection_id: connectionId }, 'failed to delete storage connection'); + return fail(500, { error: m.storage_connections_delete_failed() }); + } throw redirect(303, '/storage'); } diff --git a/src/routes/(app)/storage/page.server.test.ts b/src/routes/(app)/storage/page.server.test.ts index fe5c3d98..5ac2be20 100644 --- a/src/routes/(app)/storage/page.server.test.ts +++ b/src/routes/(app)/storage/page.server.test.ts @@ -63,7 +63,10 @@ import { load, actions } from './+page.server.js'; import { superValidate } from 'sveltekit-superforms'; function mockLocals() { - return { logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn() }, user: { id: 'test-user' } }; + return { + logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + user: { id: 'test-user' } + }; } const validFormData = { @@ -166,6 +169,31 @@ describe('storage page actions', () => { ).rejects.toThrow(expect.objectContaining({ status: 303, location: '/storage' })); }); + it('connect: returns an error message instead of throwing when saving fails', async () => { + vi.mocked(superValidate).mockResolvedValue({ + valid: true, + data: { + type: 's3', + host: 's3.example.com', + accessStyle: 'Path', + region: { name: 'us-east-1' }, + credentials: { accessKey: 'ak', secretKey: 'sk' } + } + } as unknown as Awaited>); + mockConnectionProvider.listContainers.mockResolvedValue(['b1']); + mockSaveConnection.mockRejectedValueOnce(new Error('database is locked')); + + const result = await actions.connect({ + request: new Request('http://localhost', { method: 'POST' }), + locals: mockLocals() + } as unknown as Parameters[0]); + + expect(result).toMatchObject({ + status: 500, + message: { type: 'error', message: 'Could not save the connection. Please try again.' } + }); + }); + it('disconnect: redirects to /storage', async () => { await expect( actions.disconnect({ @@ -203,7 +231,7 @@ 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: 'Connection not found.' } }); }); it('use: sets the selected connection as active and redirects', async () => { @@ -260,4 +288,22 @@ describe('storage page actions', () => { expect(mockUpdateSession).not.toHaveBeenCalled(); expect(mockDeleteConnection).toHaveBeenCalledWith('test-user', 'inactive-connection'); }); + + it('deleteConnection: returns an error instead of throwing when deleting fails', async () => { + vi.mocked(superValidate).mockResolvedValue({ + valid: true, + data: { connectionId: 'inactive-connection' } + } as unknown as Awaited>); + mockDeleteConnection.mockRejectedValueOnce(new Error('database is locked')); + + const result = await actions.deleteConnection({ + request: new Request('http://localhost', { method: 'POST' }), + locals: mockLocals() + } as unknown as Parameters[0]); + + expect(result).toMatchObject({ + status: 500, + data: { error: 'Could not delete the connection. Please try again.' } + }); + }); }); From 6fa6209402cf793a131882805ffea453ed13e03c Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Thu, 22 Oct 2026 15:01:05 +0200 Subject: [PATCH 2/3] fix(storage): address review findings on the connection forms - Only test the connection on edit when endpoint or credentials changed, so a rename still saves while the backend is unreachable - Explain connection test failures in the use action like elsewhere - Add the deleteConnection action to the edit route, which the sidebar posts to, and share the delete action body in a helper - Detect TLS protocol errors, SDK timeouts and nested causes as network errors - Skip the reload after saving an edit and drop the effect that settled the unsaved-changes dialog - Add unit tests for the edit update action --- e2e/storage/connections-management.spec.ts | 21 ++ messages/de.json | 2 +- messages/en.json | 2 +- .../storage/connection-test-error.test.ts | 18 ++ .../server/storage/connection-test-error.ts | 38 +++- .../storage/delete-connection-action.ts | 49 +++++ .../settings/connections/+page.server.ts | 45 +--- .../connections/[id]/edit/+page.server.ts | 49 +++-- .../connections/[id]/edit/+page.svelte | 11 +- .../connections/[id]/edit/page.server.test.ts | 204 ++++++++++++++++++ src/routes/(app)/storage/+page.server.ts | 46 +--- src/routes/(app)/storage/page.server.test.ts | 22 ++ 12 files changed, 401 insertions(+), 106 deletions(-) create mode 100644 src/lib/server/storage/delete-connection-action.ts create mode 100644 src/routes/(app)/settings/connections/[id]/edit/page.server.test.ts diff --git a/e2e/storage/connections-management.spec.ts b/e2e/storage/connections-management.spec.ts index 18853bab..3924e23e 100644 --- a/e2e/storage/connections-management.spec.ts +++ b/e2e/storage/connections-management.spec.ts @@ -191,6 +191,27 @@ test.describe('Storage — Connections management', () => { await expect(dataRows).toHaveCount(initialItems - 1); }); + test('sidebar on the edit page deletes a connection', async ({ page }) => { + const credentials = requireGarageCredentials(); + await connectToStorage(page, credentials); + // Disconnecting shows the saved connections sidebar; navigate from there so it stays filled. + await page.getByRole('button', { name: 'Disconnect' }).click(); + await page.getByRole('dialog').getByRole('button', { name: 'Disconnect' }).click(); + await expect(page.getByRole('heading', { name: 'Connect to storage' })).toBeVisible(); + const savedList = page.getByRole('list', { name: 'Saved connections' }); + await savedList.getByRole('button', { name: 'More options' }).first().click({ force: true }); + await page.getByRole('menuitem', { name: 'Edit', exact: true }).click(); + await waitForHydration(page); + await expect(page.getByRole('heading', { name: 'Edit connection' })).toBeVisible(); + + await savedList.getByRole('button', { name: 'More options' }).first().click({ force: true }); + await page.getByRole('menuitem', { name: 'Delete', exact: true }).click(); + await page.getByRole('dialog').getByRole('button', { name: 'Delete', exact: true }).click(); + + await expect(page).toHaveURL('/settings/connections'); + await expect(page.locator('table tbody tr')).toHaveCount(0); + }); + test('redirects to /settings/connections when editing a non-existent id', async ({ page }) => { await page.goto('/settings/connections/00000000-0000-0000-0000-000000000000/edit'); await waitForHydration(page); diff --git a/messages/de.json b/messages/de.json index d15bc568..e3729982 100644 --- a/messages/de.json +++ b/messages/de.json @@ -160,7 +160,7 @@ "storage_connect_timeout": "Zeitüberschreitung der Verbindung. Das Speicher-Backend hat nicht geantwortet.", "storage_connect_cancel": "Abbrechen", "storage_connect_error": "Verbindung fehlgeschlagen — Endpunkt und Zugangsdaten prüfen.", - "storage_connect_error_unreachable": "Verbindung zum ausgewählten Speicher nicht möglich — der Endpunkt ist möglicherweise nicht erreichbar.", + "storage_connection_switch_failed": "Wechsel zur ausgewählten Verbindung nicht möglich. Bitte erneut versuchen.", "storage_connect_error_access_denied": "Zugriff verweigert — Zugangsdaten prüfen.", "storage_connect_error_not_found": "Speicher-Backend nicht gefunden — Endpunkt prüfen.", "storage_connect_error_server_error": "Das Speicher-Backend hat einen Fehler gemeldet — Endpunkt und Zugangsdaten prüfen.", diff --git a/messages/en.json b/messages/en.json index 7b677e13..cc930ec1 100644 --- a/messages/en.json +++ b/messages/en.json @@ -160,7 +160,7 @@ "storage_connect_timeout": "Connection timed out. The storage backend did not respond.", "storage_connect_cancel": "Cancel", "storage_connect_error": "Could not connect — check the endpoint and credentials.", - "storage_connect_error_unreachable": "Could not connect to the selected storage — the endpoint may be unreachable.", + "storage_connection_switch_failed": "Could not switch to the selected connection. Please try again.", "storage_connect_error_access_denied": "Access denied — check your credentials.", "storage_connect_error_not_found": "Could not find the storage backend — check the endpoint.", "storage_connect_error_server_error": "The storage backend returned an error — check the endpoint and credentials.", diff --git a/src/lib/server/storage/connection-test-error.test.ts b/src/lib/server/storage/connection-test-error.test.ts index 6ab24ad2..2cee457e 100644 --- a/src/lib/server/storage/connection-test-error.test.ts +++ b/src/lib/server/storage/connection-test-error.test.ts @@ -55,6 +55,24 @@ describe('connectionTestErrorMessage', () => { ); }); + it('maps timeouts, TLS protocol errors and nested causes', () => { + const network = 'Could not reach the storage backend — check the host and port.'; + const timeout = Object.assign(new Error('Connection timed out after 5000 ms'), { + name: 'TimeoutError' + }); + expect(connectionTestErrorMessage(timeout)).toBe(network); + + const tls = Object.assign(new Error('write EPROTO 00000000:error:0A00010B:SSL routines'), { + code: 'EPROTO' + }); + expect(connectionTestErrorMessage(tls)).toBe(network); + + const wrapped = new Error('fetch failed', { + cause: Object.assign(new Error(''), { code: 'ECONNREFUSED' }) + }); + expect(connectionTestErrorMessage(wrapped)).toBe(network); + }); + it('falls back to the generic message', () => { expect(connectionTestErrorMessage(new Error('boom'))).toBe( 'Could not connect — check the endpoint and credentials.' diff --git a/src/lib/server/storage/connection-test-error.ts b/src/lib/server/storage/connection-test-error.ts index ce7a3b0c..0ca0a1cd 100644 --- a/src/lib/server/storage/connection-test-error.ts +++ b/src/lib/server/storage/connection-test-error.ts @@ -8,20 +8,44 @@ const NETWORK_ERROR_CODES = new Set([ 'EAI_AGAIN', 'ETIMEDOUT', 'EHOSTUNREACH', - 'ENETUNREACH' + 'ENETUNREACH', + // TLS handshake against a plain-HTTP port (https host with the wrong port). + 'EPROTO' ]); +/** The error, the members of an AggregateError, and the `cause` chain of each. */ +function relatedErrors(err: unknown, depth = 0): unknown[] { + if (!err || typeof err !== 'object' || depth > 5) return []; + const nested = [ + ...(err instanceof AggregateError ? err.errors : []), + ...('cause' in err ? [err.cause] : []) + ]; + return [err, ...nested.flatMap((e) => relatedErrors(e, depth + 1))]; +} + /** * Whether a connection failure comes from the network rather than from S3. Checks the Node error * code: connecting to `localhost` tries IPv6 and IPv4 and fails with an AggregateError whose - * message is empty, but whose code (and each inner error's code) is set. + * message is empty, but whose code (and each inner error's code) is set. The AWS SDK reports its + * connection timeout as a `TimeoutError` without a code. */ function isNetworkError(err: Error): boolean { - const errors = [err, ...(err instanceof AggregateError ? err.errors : [])]; - if (errors.some((e) => NETWORK_ERROR_CODES.has((e as { code?: unknown })?.code as string))) { - return true; - } - return /econnrefused|enotfound|eai_again|etimedout|network/.test(err.message.toLowerCase()); + return relatedErrors(err).some((e) => { + const { code, name, message } = e as { code?: unknown; name?: unknown; message?: unknown }; + if ( + typeof code === 'string' && + (NETWORK_ERROR_CODES.has(code) || code.startsWith('ERR_SSL_')) + ) { + return true; + } + if (name === 'TimeoutError') return true; + return ( + typeof message === 'string' && + /econnrefused|enotfound|eai_again|etimedout|eproto|timed out|network/.test( + message.toLowerCase() + ) + ); + }); } /** User-facing message for a failed connection test (listing the buckets of a new connection). */ diff --git a/src/lib/server/storage/delete-connection-action.ts b/src/lib/server/storage/delete-connection-action.ts new file mode 100644 index 00000000..81ca7b96 --- /dev/null +++ b/src/lib/server/storage/delete-connection-action.ts @@ -0,0 +1,49 @@ +import { fail, type ActionFailure, type RequestEvent } from '@sveltejs/kit'; +import { superValidate } from 'sveltekit-superforms'; +import { zod4 as zod } from 'sveltekit-superforms/adapters'; +import { ConnectionIdSchema } from '$lib/storage/schemas.js'; +import { auth } from '$lib/server/auth.js'; +import { deleteConnection } from './connections-db.js'; +import * as m from '$lib/paraglide/messages.js'; + +/** + * Body of the `deleteConnection` form actions: deletes the posted connection of the current user + * and clears the session's active connection if it was that one. Returns a failure to hand back + * to the form, or `undefined` on success; the caller redirects. + */ +export async function deleteConnectionAction({ + request, + locals +}: RequestEvent): Promise | undefined> { + const log = locals.logger; + const form = await superValidate(request, zod(ConnectionIdSchema)); + + if (!form.valid) { + log.debug({ errors: form.errors }, 'invalid connection id for delete'); + return fail(400, { error: m.storage_connection_edit_not_found() }); + } + + const { connectionId } = form.data; + const userId = locals.user!.id; + const activeId = locals.session?.activeStorageConnectionId ?? null; + + try { + if (connectionId === activeId) { + await auth.api.updateSession({ + headers: request.headers, + body: { activeStorageConnectionId: null } + }); + log.info( + { connection_id: connectionId }, + 'active storage connection deleted, session cleared' + ); + } + + await deleteConnection(userId, connectionId); + } catch (err) { + log.error({ err, connection_id: connectionId }, 'failed to delete storage connection'); + return fail(500, { error: m.storage_connections_delete_failed() }); + } + + return undefined; +} diff --git a/src/routes/(app)/settings/connections/+page.server.ts b/src/routes/(app)/settings/connections/+page.server.ts index 868c1596..0a88c4df 100644 --- a/src/routes/(app)/settings/connections/+page.server.ts +++ b/src/routes/(app)/settings/connections/+page.server.ts @@ -1,12 +1,8 @@ -import { fail, redirect } from '@sveltejs/kit'; +import { redirect } from '@sveltejs/kit'; import type { Actions, PageServerLoad } from './$types'; -import { superValidate } from 'sveltekit-superforms'; -import { zod4 as zod } from 'sveltekit-superforms/adapters'; import { desc, eq } from 'drizzle-orm'; -import { ConnectionIdSchema, StoredStorageConnectionSchema } from '$lib/storage/schemas.js'; -import * as m from '$lib/paraglide/messages.js'; -import { deleteConnection } from '$lib/server/storage/connections-db.js'; -import { auth } from '$lib/server/auth.js'; +import { StoredStorageConnectionSchema } from '$lib/storage/schemas.js'; +import { deleteConnectionAction } from '$lib/server/storage/delete-connection-action.js'; import { db } from '$lib/server/db.js'; import { userStorageConnections } from '$lib/server/schema.js'; import { decrypt } from '$lib/server/storage/encryption.js'; @@ -50,38 +46,9 @@ export const load: PageServerLoad = async ({ locals }) => { }; export const actions: Actions = { - deleteConnection: async ({ request, locals }) => { - const log = locals.logger; - const form = await superValidate(request, zod(ConnectionIdSchema)); - - if (!form.valid) { - log.debug({ errors: form.errors }, 'invalid connection id for delete'); - return fail(400, { error: m.storage_connection_edit_not_found() }); - } - - const { connectionId } = form.data; - const userId = locals.user!.id; - const activeId = locals.session?.activeStorageConnectionId ?? null; - - try { - if (connectionId === activeId) { - await auth.api.updateSession({ - headers: request.headers, - body: { activeStorageConnectionId: null } - }); - log.info( - { connection_id: connectionId }, - 'active storage connection deleted, session cleared' - ); - } - - await deleteConnection(userId, connectionId); - log.info({ connection_id: connectionId }, 'storage connection deleted from management page'); - } catch (err) { - log.error({ err, connection_id: connectionId }, 'failed to delete storage connection'); - return fail(500, { error: m.storage_connections_delete_failed() }); - } - + deleteConnection: async (event) => { + const failure = await deleteConnectionAction(event); + if (failure) return failure; throw redirect(303, '/settings/connections'); } }; diff --git a/src/routes/(app)/settings/connections/[id]/edit/+page.server.ts b/src/routes/(app)/settings/connections/[id]/edit/+page.server.ts index 49af8b8e..da4e6c4d 100644 --- a/src/routes/(app)/settings/connections/[id]/edit/+page.server.ts +++ b/src/routes/(app)/settings/connections/[id]/edit/+page.server.ts @@ -17,6 +17,7 @@ import { db } from '$lib/server/db.js'; import { userStorageConnections } from '$lib/server/schema.js'; import { decrypt, encrypt, fingerprint } from '$lib/server/storage/encryption.js'; import { isConnectionNameConflict } from '$lib/server/storage/connections-db.js'; +import { deleteConnectionAction } from '$lib/server/storage/delete-connection-action.js'; import { storageEncryptionKey } from '$lib/server/storage/encryption-key.js'; import * as m from '$lib/paraglide/messages.js'; @@ -102,9 +103,13 @@ export const actions: Actions = { } let existing: z.infer; + let storedFingerprint: string; try { const rows = await db - .select({ encryptedPayload: userStorageConnections.encryptedPayload }) + .select({ + encryptedPayload: userStorageConnections.encryptedPayload, + hash: userStorageConnections.hash + }) .from(userStorageConnections) .where( and( @@ -124,6 +129,7 @@ export const actions: Actions = { existing = StoredStorageConnectionSchema.parse( JSON.parse(decrypt(rows[0].encryptedPayload, storageEncryptionKey())) ); + storedFingerprint = rows[0].hash; } catch (err) { log.error( { err, connection_id: connectionId }, @@ -158,18 +164,6 @@ export const actions: Actions = { credentials: resolvedCredentials?.accessKey ? resolvedCredentials : undefined }; - try { - await getConnectionProvider(config).listContainers(); - log.info({ connection_id: connectionId }, 'storage connection edit verified'); - } catch (err) { - log.warn({ err, connection_id: connectionId }, 'storage connection edit test failed'); - return message( - form, - { type: 'error', message: connectionTestErrorMessage(err) }, - { status: 400 } - ); - } - const payload: z.infer = { host, port, @@ -179,7 +173,7 @@ export const actions: Actions = { credentials: resolvedCredentials }; const encryptedPayload = encrypt(JSON.stringify(payload), storageEncryptionKey()); - const hash = fingerprint( + const newFingerprint = fingerprint( { endpoint: host, port, @@ -192,12 +186,30 @@ export const actions: Actions = { storageEncryptionKey() ); + // Only test the connection when the endpoint or credentials changed: a rename must not fail + // because the backend is down at the moment. + if (newFingerprint === storedFingerprint) { + log.debug({ connection_id: connectionId }, 'connection settings unchanged, skipping test'); + } else { + try { + await getConnectionProvider(config).listContainers(); + log.info({ connection_id: connectionId }, 'storage connection edit verified'); + } catch (err) { + log.warn({ err, connection_id: connectionId }, 'storage connection edit test failed'); + return message( + form, + { type: 'error', message: connectionTestErrorMessage(err) }, + { status: 400 } + ); + } + } + // A blank name keeps the current one; the name column is required. const name = form.data.name?.trim() || undefined; try { await db .update(userStorageConnections) - .set({ encryptedPayload, hash, name }) + .set({ encryptedPayload, hash: newFingerprint, name }) .where( and( eq(userStorageConnections.id, connectionId), @@ -220,5 +232,12 @@ export const actions: Actions = { log.info({ connection_id: connectionId }, 'storage connection updated'); return message(form, { type: 'success' }); + }, + + // The connection sidebar on this page posts its delete forms here. + deleteConnection: async (event) => { + const failure = await deleteConnectionAction(event); + if (failure) return failure; + throw redirect(303, '/settings/connections'); } }; diff --git a/src/routes/(app)/settings/connections/[id]/edit/+page.svelte b/src/routes/(app)/settings/connections/[id]/edit/+page.svelte index e590335e..948f367f 100644 --- a/src/routes/(app)/settings/connections/[id]/edit/+page.svelte +++ b/src/routes/(app)/settings/connections/[id]/edit/+page.svelte @@ -29,9 +29,10 @@ } // Escape or a backdrop click closes the dialog without an answer: that means stay. - $effect(() => { - if (!confirmLeaveOpen) closeLeaveDialog(false); - }); + function setLeaveDialogOpen(open: boolean) { + if (open) confirmLeaveOpen = true; + else closeLeaveDialog(false); + } const { form, errors, enhance, submitting, message } = superForm( untrack(() => data.editForm), @@ -40,6 +41,8 @@ validators: zod(EditStorageConnectionSchema), // Keep the entered values until the redirect instead of flashing the defaults. resetForm: false, + // A successful save navigates away, so reloading this page's data would be wasted. + invalidateAll: false, taintedMessage: () => new Promise((settle) => { answerLeave = settle; @@ -71,7 +74,7 @@ } - + confirmLeaveOpen, setLeaveDialogOpen}>

{m.storage_connection_edit_unsaved_title()}

{m.storage_connection_edit_unsaved_body()}

diff --git a/src/routes/(app)/settings/connections/[id]/edit/page.server.test.ts b/src/routes/(app)/settings/connections/[id]/edit/page.server.test.ts new file mode 100644 index 00000000..7de6b1c4 --- /dev/null +++ b/src/routes/(app)/settings/connections/[id]/edit/page.server.test.ts @@ -0,0 +1,204 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mockConnectionProvider = { listContainers: vi.fn() }; +const mockGetConnectionProvider = vi.fn((config: unknown) => { + void config; + return mockConnectionProvider; +}); +vi.mock('$lib/server/storage/utils.js', () => ({ + getConnectionProvider: (config: unknown) => mockGetConnectionProvider(config) +})); + +vi.mock('$lib/storage/schemas.js', () => ({ + EditStorageConnectionSchema: {}, + ConnectionIdSchema: {}, + StoredStorageConnectionSchema: { parse: (value: unknown) => value } +})); + +vi.mock('$lib/server/auth.js', () => ({ + auth: { api: { updateSession: vi.fn() } } +})); + +vi.mock('$lib/server/storage/connections-db.js', () => ({ + isConnectionNameConflict: () => false, + deleteConnection: vi.fn() +})); + +vi.mock('sveltekit-superforms', () => ({ + superValidate: vi.fn(), + message: vi.fn((form, msg, opts) => ({ form, message: msg, status: opts?.status })), + setError: vi.fn((form, field, msg) => ({ form, field, error: msg })) +})); + +vi.mock('sveltekit-superforms/adapters', () => ({ + zod4: vi.fn((schema) => schema) +})); + +const mockDbSelect = vi.fn(); +const mockDbUpdate = vi.fn().mockResolvedValue(undefined); +vi.mock('$lib/server/db.js', () => ({ + db: { + select: () => ({ from: () => ({ where: () => ({ limit: mockDbSelect }) }) }), + update: () => ({ set: () => ({ where: mockDbUpdate }) }) + } +})); + +const mockFingerprint = vi.fn(() => 'changed-hash'); +vi.mock('$lib/server/storage/encryption.js', () => ({ + encrypt: vi.fn(() => 'encrypted-payload'), + decrypt: vi.fn((payload: string) => payload), + fingerprint: () => mockFingerprint() +})); + +vi.mock('$lib/server/storage/encryption-key.js', () => ({ + storageEncryptionKey: () => Buffer.alloc(32) +})); + +vi.mock('$lib/server/schema.js', () => ({ + userStorageConnections: {} +})); + +import { actions } from './+page.server.js'; +import { superValidate } from 'sveltekit-superforms'; + +const storedCredentials = { accessKey: 'stored-ak', secretKey: 'stored-sk' }; + +const storedPayload = { + host: 's3.example.com', + port: 9000, + accessStyle: 'Path', + region: { name: 'us-east-1' }, + credentials: storedCredentials +}; + +function mockForm(credentials = { accessKey: '', secretKey: '' }) { + vi.mocked(superValidate).mockResolvedValue({ + valid: true, + data: { + id: 'conn-1', + name: 'Renamed', + type: 's3', + host: 's3.example.com', + port: 9000, + accessStyle: 'Path', + region: { name: 'us-east-1' }, + credentials + } + } as unknown as Awaited>); +} + +function mockStoredRow(payload: object = storedPayload) { + mockDbSelect.mockResolvedValue([ + { encryptedPayload: JSON.stringify(payload), hash: 'stored-hash' } + ]); +} + +function update() { + return actions.update({ + request: new Request('http://localhost', { method: 'POST' }), + params: { id: 'conn-1' }, + locals: { + logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + user: { id: 'test-user' } + } + } as unknown as Parameters[0]); +} + +describe('connection edit update action', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockFingerprint.mockReturnValue('changed-hash'); + mockConnectionProvider.listContainers.mockResolvedValue(['bucket']); + mockDbUpdate.mockResolvedValue(undefined); + }); + + it('tests the connection with the stored credentials when the fields are blank', async () => { + mockForm(); + mockStoredRow(); + + const result = await update(); + + expect(mockGetConnectionProvider).toHaveBeenCalledWith( + expect.objectContaining({ credentials: storedCredentials }) + ); + expect(result).toMatchObject({ message: { type: 'success' } }); + }); + + it('tests a connection without stored credentials anonymously', async () => { + mockForm(); + mockStoredRow({ ...storedPayload, credentials: undefined }); + + await update(); + + expect(mockGetConnectionProvider).toHaveBeenCalledWith( + expect.objectContaining({ credentials: undefined }) + ); + }); + + it('skips the connection test when only the name changed', async () => { + mockForm(); + mockStoredRow(); + mockFingerprint.mockReturnValue('stored-hash'); + mockConnectionProvider.listContainers.mockRejectedValue(new Error('backend down')); + + const result = await update(); + + expect(mockConnectionProvider.listContainers).not.toHaveBeenCalled(); + expect(mockDbUpdate).toHaveBeenCalled(); + expect(result).toMatchObject({ message: { type: 'success' } }); + }); + + it('does not save when the connection test of changed settings fails', async () => { + mockForm({ accessKey: 'new-ak', secretKey: 'new-sk' }); + mockStoredRow(); + mockConnectionProvider.listContainers.mockRejectedValue( + Object.assign(new Error(''), { code: 'ECONNREFUSED' }) + ); + + const result = await update(); + + expect(mockDbUpdate).not.toHaveBeenCalled(); + expect(result).toMatchObject({ + status: 400, + message: { + type: 'error', + message: 'Could not reach the storage backend — check the host and port.' + } + }); + }); + + it('requires the secret key when the access key changed', async () => { + mockForm({ accessKey: 'new-ak', secretKey: '' }); + mockStoredRow(); + + const result = await update(); + + expect(mockConnectionProvider.listContainers).not.toHaveBeenCalled(); + expect(result).toMatchObject({ field: 'credentials.secretKey' }); + }); + + it('returns a not-found message for a missing row', async () => { + mockForm(); + mockDbSelect.mockResolvedValue([]); + + const result = await update(); + + expect(result).toMatchObject({ + status: 404, + message: { type: 'error', message: 'Connection not found.' } + }); + }); + + it('returns an error message instead of throwing when the update fails', async () => { + mockForm(); + mockStoredRow(); + mockDbUpdate.mockRejectedValueOnce(new Error('database is locked')); + + const result = await update(); + + expect(result).toMatchObject({ + status: 500, + message: { type: 'error', message: 'Could not save the connection. Please try again.' } + }); + }); +}); diff --git a/src/routes/(app)/storage/+page.server.ts b/src/routes/(app)/storage/+page.server.ts index 3afa3ccf..bacd7fcc 100644 --- a/src/routes/(app)/storage/+page.server.ts +++ b/src/routes/(app)/storage/+page.server.ts @@ -12,11 +12,8 @@ import { } from '$lib/storage/schemas.js'; import { getConnectionProvider } from '$lib/server/storage/utils.js'; import { connectionTestErrorMessage } from '$lib/server/storage/connection-test-error.js'; -import { - saveConnection, - getConnectionForUser, - deleteConnection -} from '$lib/server/storage/connections-db.js'; +import { saveConnection, getConnectionForUser } from '$lib/server/storage/connections-db.js'; +import { deleteConnectionAction } from '$lib/server/storage/delete-connection-action.js'; import { auth } from '$lib/server/auth.js'; import { db } from '$lib/server/db.js'; import { userStorageConnections } from '$lib/server/schema.js'; @@ -164,7 +161,7 @@ export const actions: Actions = { await getConnectionProvider(config).listContainers(); } catch (err) { log.warn({ err, connection_id: connectionId }, 'storage connection test failed on use'); - return fail(400, { error: m.storage_connect_error_unreachable() }); + return fail(400, { error: connectionTestErrorMessage(err) }); } await auth.api.updateSession({ @@ -174,44 +171,15 @@ export const actions: Actions = { log.info({ connection_id: connectionId }, 'user switched storage connection'); } catch (err) { log.error({ err, connection_id: connectionId }, 'failed to switch storage connection'); - return fail(500, { error: m.storage_connect_error() }); + return fail(500, { error: m.storage_connection_switch_failed() }); } throw redirect(303, '/storage'); }, - deleteConnection: async ({ request, locals }) => { - const log = locals.logger; - const form = await superValidate(request, zod(ConnectionIdSchema)); - - if (!form.valid) { - log.debug({ errors: form.errors }, 'invalid connection id for delete'); - return fail(400, { error: m.storage_connection_edit_not_found() }); - } - - const { connectionId } = form.data; - const userId = locals.user!.id; - const activeId = locals.session?.activeStorageConnectionId ?? null; - - try { - if (connectionId === activeId) { - await auth.api.updateSession({ - headers: request.headers, - body: { activeStorageConnectionId: null } - }); - log.info( - { connection_id: connectionId }, - 'active storage connection deleted, session cleared' - ); - } - - await deleteConnection(userId, connectionId); - log.info({ connection_id: connectionId }, 'storage connection deleted'); - } catch (err) { - log.error({ err, connection_id: connectionId }, 'failed to delete storage connection'); - return fail(500, { error: m.storage_connections_delete_failed() }); - } - + deleteConnection: async (event) => { + const failure = await deleteConnectionAction(event); + if (failure) return failure; throw redirect(303, '/storage'); } }; diff --git a/src/routes/(app)/storage/page.server.test.ts b/src/routes/(app)/storage/page.server.test.ts index 5ac2be20..13182da1 100644 --- a/src/routes/(app)/storage/page.server.test.ts +++ b/src/routes/(app)/storage/page.server.test.ts @@ -234,6 +234,28 @@ describe('storage page actions', () => { expect(result).toMatchObject({ status: 400, data: { error: 'Connection not found.' } }); }); + it('use: explains why the connection test failed', async () => { + vi.mocked(superValidate).mockResolvedValue({ + valid: true, + data: { connectionId: 'saved-connection' } + } as unknown as Awaited>); + mockGetConnectionForUser.mockResolvedValue(validFormData); + mockConnectionProvider.listContainers.mockRejectedValue( + Object.assign(new Error(''), { code: '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 () => { vi.mocked(superValidate).mockResolvedValue({ valid: true, From 4d4c6e6dd92cf757aeef26ede3804285e80d27d7 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Thu, 22 Oct 2026 15:54:17 +0200 Subject: [PATCH 3/3] fix(storage): handle sidebar delete and activation failures - Show a failed sidebar delete on the connection edit page - Save and activate a new connection in separate steps, so an activation failure no longer claims the connection was not saved - Send sidebar deletes with enhance and ask about unsaved changes before the delete is sent, in the app instead of the browser's leave prompt; a failed delete brings the unsaved changes back - Drop deleted connections from the edit page sidebar - Wait for the list to change in the e2e cleanup helper now that deletes no longer reload the page - Add e2e tests for the edit page sidebar: switching connections, delete with and without unsaved changes, failed delete --- e2e/storage/connections-management.spec.ts | 166 +++++++++++++++++- e2e/storage/helpers.ts | 23 ++- messages/de.json | 1 + messages/en.json | 1 + .../sidebar/StorageConnectionSidebar.svelte | 39 +++- src/lib/storage/context.ts | 14 ++ .../settings/connections/[id]/+layout.svelte | 18 ++ .../connections/[id]/edit/+page.svelte | 42 ++++- src/routes/(app)/storage/+page.server.ts | 17 +- src/routes/(app)/storage/page.server.test.ts | 30 ++++ 10 files changed, 330 insertions(+), 21 deletions(-) diff --git a/e2e/storage/connections-management.spec.ts b/e2e/storage/connections-management.spec.ts index 3924e23e..4dea636c 100644 --- a/e2e/storage/connections-management.spec.ts +++ b/e2e/storage/connections-management.spec.ts @@ -1,5 +1,10 @@ import { test, expect, type Page } from '../support/fixtures.js'; -import { hasGarageCredentials, requireGarageCredentials } from '../support/garage.js'; +import { + createGarageBucketCredentials, + hasGarageAdmin, + hasGarageCredentials, + requireGarageCredentials +} from '../support/garage.js'; import { connectToStorage, openConnectForm } from './helpers.js'; import { waitForHydration } from '../support/helpers.js'; @@ -321,3 +326,162 @@ test.describe('Storage — Connections management', () => { await expect(page).toHaveURL('/settings/connections'); }); }); + +test.describe('Storage — Connection edit sidebar', () => { + test.use({ locale: 'en-US' }); + + test.beforeEach(() => { + test.skip( + !hasGarageCredentials() || !hasGarageAdmin(), + 'Skipped: requires a running Garage instance with admin access' + ); + }); + + /** + * Saves two connections that differ in their access key, then opens the edit page of the most + * recent one from the saved connections sidebar, which stays filled when navigated from there. + * Returns the access keys in sidebar order. + */ + async function openEditWithTwoConnections(page: Page): Promise<[string, string]> { + const base = requireGarageCredentials(); + const second = await createGarageBucketCredentials(base, { + bucketName: 'e2e-connection-edit-sidebar', + keyName: 'e2e-connection-edit-sidebar', + permissions: { owner: false, read: true, write: false } + }); + await connectToStorage(page, base); + await connectToStorage(page, second, { clearSaved: false }); + + await page.getByRole('button', { name: 'Disconnect' }).click(); + await page.getByRole('dialog').getByRole('button', { name: 'Disconnect' }).click(); + await expect(page.getByRole('heading', { name: 'Connect to storage' })).toBeVisible(); + const savedList = page.getByRole('list', { name: 'Saved connections' }); + await expect(savedList.getByRole('listitem')).toHaveCount(2); + await savedList.getByRole('button', { name: 'More options' }).first().click({ force: true }); + await page.getByRole('menuitem', { name: 'Edit', exact: true }).click(); + await waitForHydration(page); + await expect(page.getByRole('heading', { name: 'Edit connection' })).toBeVisible(); + + // Most recently saved first. + return [second.accessKeyId, base.accessKeyId]; + } + + function sidebarConnection(page: Page, index: number) { + return page + .getByRole('list', { name: 'Saved connections' }) + .getByRole('listitem') + .nth(index) + .getByRole('button') + .first(); + } + + async function deleteFromSidebar(page: Page, index: number) { + await page + .getByRole('list', { name: 'Saved connections' }) + .getByRole('button', { name: 'More options' }) + .nth(index) + .click({ force: true }); + await page.getByRole('menuitem', { name: 'Delete', exact: true }).click(); + await page.getByRole('dialog').getByRole('button', { name: 'Delete', exact: true }).click(); + } + + test('switching connections loads the other connection without asking', async ({ page }) => { + const [firstKey, secondKey] = await openEditWithTwoConnections(page); + await expect(page.getByLabel('Access key')).toHaveValue(firstKey); + const firstUrl = page.url(); + + await sidebarConnection(page, 1).click(); + + await expect(page).not.toHaveURL(firstUrl); + await expect(page.getByLabel('Access key')).toHaveValue(secondKey); + await expect(page.getByRole('dialog')).toHaveCount(0); + + // The switch must not leave the form tainted. + await page.getByRole('link', { name: '← Manage connections' }).click(); + await expect(page).toHaveURL('/settings/connections'); + await expect(page.getByRole('dialog')).toHaveCount(0); + }); + + test('switching connections with unsaved changes asks first', async ({ page }) => { + await openEditWithTwoConnections(page); + const firstUrl = page.url(); + + await page.getByLabel('Connection name').fill('Changed name'); + await sidebarConnection(page, 1).click(); + + await expect(page.getByRole('heading', { name: 'Unsaved changes' })).toBeVisible(); + await page.getByRole('dialog').getByRole('button', { name: 'Stay on page' }).click(); + await expect(page).toHaveURL(firstUrl); + await expect(page.getByLabel('Connection name')).toHaveValue('Changed name'); + }); + + test('sidebar delete with unsaved changes asks in the app before deleting', async ({ page }) => { + await openEditWithTwoConnections(page); + let deleteRequests = 0; + let nativeDialogs = 0; + page.on('request', (request) => { + if (request.url().endsWith('?/deleteConnection')) deleteRequests++; + }); + page.on('dialog', (dialog) => { + nativeDialogs++; + void dialog.dismiss(); + }); + + await page.getByLabel('Connection name').fill('Changed name'); + await deleteFromSidebar(page, 1); + + await expect(page.getByRole('heading', { name: 'Unsaved changes' })).toBeVisible(); + await page.getByRole('button', { name: 'Stay on page' }).click(); + await expect(page.getByRole('heading', { name: 'Unsaved changes' })).toBeHidden(); + await expect(page).toHaveURL(/\/edit$/); + expect(deleteRequests).toBe(0); + + await deleteFromSidebar(page, 1); + await page.getByRole('button', { name: 'Leave' }).click(); + + await expect(page).toHaveURL('/settings/connections'); + await expect(page.locator('table tbody tr')).toHaveCount(1); + expect(deleteRequests).toBe(1); + expect(nativeDialogs).toBe(0); + }); + + test('sidebar delete without changes deletes without asking', async ({ page }) => { + await openEditWithTwoConnections(page); + + await deleteFromSidebar(page, 1); + + await expect(page).toHaveURL('/settings/connections'); + await expect(page.getByRole('heading', { name: 'Unsaved changes' })).toHaveCount(0); + await expect(page.locator('table tbody tr')).toHaveCount(1); + + // The sidebar on the next edit page no longer lists the deleted connection. + await openFirstConnectionEditPage(page); + await expect( + page.getByRole('list', { name: 'Saved connections' }).getByRole('listitem') + ).toHaveCount(1); + }); + + test('a failed sidebar delete shows the error and keeps the unsaved changes', async ({ + page + }) => { + await openEditWithTwoConnections(page); + // An invalid ID makes the action fail. + await page.route(/\?\/deleteConnection$/, (route) => + route.continue({ postData: 'connectionId=not-a-uuid' }) + ); + + await page.getByLabel('Connection name').fill('Changed name'); + await deleteFromSidebar(page, 1); + await page.getByRole('button', { name: 'Leave' }).click(); + + await expect( + page.getByRole('alert').filter({ hasText: 'Connection not found.' }) + ).toBeVisible(); + await expect(page).toHaveURL(/\/edit$/); + await expect(page.getByLabel('Connection name')).toHaveValue('Changed name'); + + // The changes are still guarded. + await page.getByRole('link', { name: '← Manage connections' }).click(); + await expect(page.getByRole('heading', { name: 'Unsaved changes' })).toBeVisible(); + }); +}); diff --git a/e2e/storage/helpers.ts b/e2e/storage/helpers.ts index 0b40a6c6..02a7fe25 100644 --- a/e2e/storage/helpers.ts +++ b/e2e/storage/helpers.ts @@ -53,6 +53,7 @@ async function clearSavedConnections(page: Page) { .catch(() => false) ) break; + const before = await items.count(); // Click the last button in the first list item. On desktop this is the // "more options" button (opacity-0, so use force). On mobile it's the // X delete button which submits the form directly. @@ -65,9 +66,15 @@ async function clearSavedConnections(page: Page) { // Confirm in the modal await page.getByRole('button', { name: 'Delete', exact: true }).click(); } - // The form action POSTs and the server redirects back to /storage; - // wait for the page to re-hydrate before checking the list again. - await waitForHydration(page); + // The delete is sent without a page reload: wait until the list has changed. The last + // delete empties it (desktop shows a placeholder, mobile hides the list). + await expect(async () => { + const changed = + !(await savedList.isVisible()) || + (await savedList.getByText('No saved connections yet').isVisible()) || + (await items.count()) < before; + expect(changed).toBe(true); + }).toPass(); } } @@ -103,14 +110,18 @@ export async function openConnectForm(page: Page, { clearSaved = true } = {}) { } } -export async function connectToStorage(page: Page, credentials: GarageCredentials) { - await openConnectForm(page); +export async function connectToStorage( + page: Page, + credentials: GarageCredentials, + { clearSaved = true } = {} +) { + await openConnectForm(page, { clearSaved }); const url = new URL(credentials.endpoint); const host = url.hostname; const port = url.port; const useTls = url.protocol === 'https:'; - await page.getByLabel('Host').fill(host); + await page.getByLabel('Host', { exact: true }).fill(host); if (port) { await page.getByLabel('Port').fill(port); } diff --git a/messages/de.json b/messages/de.json index e3729982..51c70bfa 100644 --- a/messages/de.json +++ b/messages/de.json @@ -167,6 +167,7 @@ "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_save_failed": "Die Verbindung konnte nicht gespeichert werden. Bitte erneut versuchen.", + "storage_connect_activate_failed": "Die Verbindung wurde gespeichert, konnte aber nicht aktiviert werden. Laden Sie die Seite neu und wählen Sie sie in den gespeicherten Verbindungen aus.", "storage_connect_host_required": "Host ist erforderlich", "storage_connect_host_invalid": "Host muss ein gültiger Hostname oder eine gültige IP-Adresse sein", "storage_connect_port_invalid": "Port muss eine ganze Zahl zwischen 1 und 65535 sein", diff --git a/messages/en.json b/messages/en.json index cc930ec1..5ab0a133 100644 --- a/messages/en.json +++ b/messages/en.json @@ -167,6 +167,7 @@ "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_save_failed": "Could not save the connection. Please try again.", + "storage_connect_activate_failed": "The connection was saved but could not be activated. Reload the page and select it from the saved connections.", "storage_connect_host_required": "Host is required", "storage_connect_host_invalid": "Host must be a valid hostname or IP address", "storage_connect_port_invalid": "Port must be a whole number between 1 and 65535", diff --git a/src/lib/components/storage/sidebar/StorageConnectionSidebar.svelte b/src/lib/components/storage/sidebar/StorageConnectionSidebar.svelte index 0e815be0..92ccb314 100644 --- a/src/lib/components/storage/sidebar/StorageConnectionSidebar.svelte +++ b/src/lib/components/storage/sidebar/StorageConnectionSidebar.svelte @@ -6,7 +6,9 @@ import TooltipTrigger from '$lib/components/TooltipTrigger.svelte'; import { createResizablePanel } from './resizable-panel.svelte.js'; import ResizeHandle from './ResizeHandle.svelte'; + import { enhance } from '$app/forms'; import { resolve } from '$app/paths'; + import type { SubmitFunction } from '@sveltejs/kit'; import * as m from '$lib/paraglide/messages.js'; import type { ConnectionMetadata } from '$lib/storage/connection-types.js'; import { connectionLabel } from '$lib/storage/display-helpers.js'; @@ -25,9 +27,16 @@ * connection edits rather than activating them. */ onselect?: (conn: ConnectionMetadata) => void; + /** + * Called before a delete is sent. Resolving `false` cancels it. Edit pages use this to ask + * about unsaved changes before the connection is deleted, not after. + */ + onbeforedelete?: () => Promise; + /** Called once the server has answered a delete; `deleted` is false when it failed. */ + ondeletesettled?: (connectionId: string, deleted: boolean) => void; } - let { connections, activeId, onselect }: Props = $props(); + let { connections, activeId, onselect, onbeforedelete, ondeletesettled }: Props = $props(); const resize = createResizablePanel({ storageKey: 'storage_connect_sidebar_width', @@ -96,6 +105,22 @@ closeContextMenu(); } + const enhanceDelete: SubmitFunction = async ({ cancel }) => { + if (onbeforedelete && !(await onbeforedelete())) { + cancel(); + cancelDelete(); + return; + } + return async ({ formData, result, update }) => { + cancelDelete(); + ondeletesettled?.( + String(formData.get('connectionId')), + result.type === 'redirect' || result.type === 'success' + ); + await update(); + }; + }; + const deleteConn = $derived.by(() => { if (!menuConn) return null; const ep = menuConn.endpoint ?? ''; @@ -111,7 +136,13 @@ {#if menuConn} -
+
    { - // Redirect handles the page update - }} + use:enhance={enhanceDelete} class="absolute -top-2 -right-2" > diff --git a/src/lib/storage/context.ts b/src/lib/storage/context.ts index 4d692899..d75bf345 100644 --- a/src/lib/storage/context.ts +++ b/src/lib/storage/context.ts @@ -4,3 +4,17 @@ import type { TabsState } from './tabs.svelte.js'; export const [getStorageState, setStorageState] = createContext(); export const [getTabsState, setTabsState] = createContext(); + +/** + * Lets the connection edit page ask about unsaved changes before the sidebar in its layout + * deletes a connection. + */ +export interface ConnectionEditGuard { + /** Resolves `true` when the delete may go ahead; the changes are then discarded. */ + confirmDiscard: (() => Promise) | null; + /** Brings back the discarded changes when the delete failed. */ + restoreChanges: (() => void) | null; +} + +export const [getConnectionEditGuard, setConnectionEditGuard] = + createContext(); diff --git a/src/routes/(app)/settings/connections/[id]/+layout.svelte b/src/routes/(app)/settings/connections/[id]/+layout.svelte index 6ff76e73..3e027772 100644 --- a/src/routes/(app)/settings/connections/[id]/+layout.svelte +++ b/src/routes/(app)/settings/connections/[id]/+layout.svelte @@ -5,14 +5,30 @@ import StorageConnectionSidebar from '$lib/components/storage/sidebar/StorageConnectionSidebar.svelte'; import type { ConnectionMetadata } from '$lib/storage/connection-types.js'; import { connectionStore } from '$lib/storage/connection-store.svelte.js'; + import { setConnectionEditGuard, type ConnectionEditGuard } from '$lib/storage/context.js'; let { children } = $props(); const connectionId = $derived(page.params.id); + // The edit page registers its unsaved-changes check here. + const editGuard: ConnectionEditGuard = { confirmDiscard: null, restoreChanges: null }; + setConnectionEditGuard(editGuard); + function handleSelect(conn: ConnectionMetadata) { goto(resolve(`/settings/connections/${conn.id}/edit`)); } + + // Only /storage fills the store, so drop deleted connections here. + function handleDeleteSettled(connectionId: string, deleted: boolean) { + if (deleted) { + connectionStore.connections = connectionStore.connections.filter( + (c) => c.id !== connectionId + ); + } else { + editGuard.restoreChanges?.(); + } + }
    @@ -20,6 +36,8 @@ connections={connectionStore.connections} activeId={connectionId} onselect={handleSelect} + onbeforedelete={() => editGuard.confirmDiscard?.() ?? Promise.resolve(true)} + ondeletesettled={handleDeleteSettled} />
    {@render children()} diff --git a/src/routes/(app)/settings/connections/[id]/edit/+page.svelte b/src/routes/(app)/settings/connections/[id]/edit/+page.svelte index 948f367f..9f35ca49 100644 --- a/src/routes/(app)/settings/connections/[id]/edit/+page.svelte +++ b/src/routes/(app)/settings/connections/[id]/edit/+page.svelte @@ -2,6 +2,7 @@ import { untrack } from 'svelte'; import { goto } from '$app/navigation'; import { resolve } from '$app/paths'; + import { page } from '$app/state'; import { superForm } from 'sveltekit-superforms'; import { zod4Client as zod } from 'sveltekit-superforms/adapters'; import IconInfo from 'virtual:icons/material-symbols/info'; @@ -11,6 +12,7 @@ EditStorageConnectionSchema, type StorageConnectionMessage } from '$lib/storage/schemas.js'; + import { getConnectionEditGuard } from '$lib/storage/context.js'; let { data } = $props(); @@ -18,6 +20,9 @@ let isActiveConnection = $derived(data.activeConnectionId === data.connectionId); + // Set when the sidebar's delete fails; a successful delete redirects. + const deleteError = $derived((page.form as { error?: string } | null)?.error ?? null); + // Unsaved-changes guard: superforms asks before leaving a tainted form, the modal answers. let confirmLeaveOpen = $state(false); let answerLeave: ((leave: boolean) => void) | null = null; @@ -34,7 +39,14 @@ else closeLeaveDialog(false); } - const { form, errors, enhance, submitting, message } = superForm( + function askLeave(): Promise { + return new Promise((settle) => { + answerLeave = settle; + confirmLeaveOpen = true; + }); + } + + const { form, errors, enhance, submitting, message, tainted, isTainted } = superForm( untrack(() => data.editForm), { dataType: 'json', @@ -43,11 +55,7 @@ resetForm: false, // A successful save navigates away, so reloading this page's data would be wasted. invalidateAll: false, - taintedMessage: () => - new Promise((settle) => { - answerLeave = settle; - confirmLeaveOpen = true; - }), + taintedMessage: askLeave, onUpdated({ form }) { if ((form.message as StorageConnectionMessage | undefined)?.type === 'success') { void goto(resolve('/settings/connections')); @@ -60,6 +68,24 @@ } ); + // A sidebar delete navigates away: ask before it is sent, then drop the changes so the + // redirect does not ask again. A failed delete brings them back. + const editGuard = getConnectionEditGuard(); + let discardedChanges: Parameters[0] = undefined; + editGuard.confirmDiscard = async () => { + if (!isTainted()) return true; + const leave = await askLeave(); + if (leave) { + discardedChanges = $tainted; + tainted.set(undefined); + } + return leave; + }; + editGuard.restoreChanges = () => { + if (discardedChanges) tainted.set(discardedChanges); + discardedChanges = undefined; + }; + function parseHostInput() { const v = $form.host?.trim(); if (!v?.includes('://')) return; @@ -91,6 +117,10 @@

    {m.storage_connection_edit_title()}

    +{#if deleteError} + +{/if} + {#if isActiveConnection}