Skip to content

Commit 4d4c6e6

Browse files
committed
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
1 parent 6fa6209 commit 4d4c6e6

10 files changed

Lines changed: 330 additions & 21 deletions

File tree

‎e2e/storage/connections-management.spec.ts‎

Lines changed: 165 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
import { test, expect, type Page } from '../support/fixtures.js';
2-
import { hasGarageCredentials, requireGarageCredentials } from '../support/garage.js';
2+
import {
3+
createGarageBucketCredentials,
4+
hasGarageAdmin,
5+
hasGarageCredentials,
6+
requireGarageCredentials
7+
} from '../support/garage.js';
38
import { connectToStorage, openConnectForm } from './helpers.js';
49
import { waitForHydration } from '../support/helpers.js';
510

@@ -321,3 +326,162 @@ test.describe('Storage — Connections management', () => {
321326
await expect(page).toHaveURL('/settings/connections');
322327
});
323328
});
329+
330+
test.describe('Storage — Connection edit sidebar', () => {
331+
test.use({ locale: 'en-US' });
332+
333+
test.beforeEach(() => {
334+
test.skip(
335+
!hasGarageCredentials() || !hasGarageAdmin(),
336+
'Skipped: requires a running Garage instance with admin access'
337+
);
338+
});
339+
340+
/**
341+
* Saves two connections that differ in their access key, then opens the edit page of the most
342+
* recent one from the saved connections sidebar, which stays filled when navigated from there.
343+
* Returns the access keys in sidebar order.
344+
*/
345+
async function openEditWithTwoConnections(page: Page): Promise<[string, string]> {
346+
const base = requireGarageCredentials();
347+
const second = await createGarageBucketCredentials(base, {
348+
bucketName: 'e2e-connection-edit-sidebar',
349+
keyName: 'e2e-connection-edit-sidebar',
350+
permissions: { owner: false, read: true, write: false }
351+
});
352+
await connectToStorage(page, base);
353+
await connectToStorage(page, second, { clearSaved: false });
354+
355+
await page.getByRole('button', { name: 'Disconnect' }).click();
356+
await page.getByRole('dialog').getByRole('button', { name: 'Disconnect' }).click();
357+
await expect(page.getByRole('heading', { name: 'Connect to storage' })).toBeVisible();
358+
const savedList = page.getByRole('list', { name: 'Saved connections' });
359+
await expect(savedList.getByRole('listitem')).toHaveCount(2);
360+
await savedList.getByRole('button', { name: 'More options' }).first().click({ force: true });
361+
await page.getByRole('menuitem', { name: 'Edit', exact: true }).click();
362+
await waitForHydration(page);
363+
await expect(page.getByRole('heading', { name: 'Edit connection' })).toBeVisible();
364+
365+
// Most recently saved first.
366+
return [second.accessKeyId, base.accessKeyId];
367+
}
368+
369+
function sidebarConnection(page: Page, index: number) {
370+
return page
371+
.getByRole('list', { name: 'Saved connections' })
372+
.getByRole('listitem')
373+
.nth(index)
374+
.getByRole('button')
375+
.first();
376+
}
377+
378+
async function deleteFromSidebar(page: Page, index: number) {
379+
await page
380+
.getByRole('list', { name: 'Saved connections' })
381+
.getByRole('button', { name: 'More options' })
382+
.nth(index)
383+
.click({ force: true });
384+
await page.getByRole('menuitem', { name: 'Delete', exact: true }).click();
385+
await page.getByRole('dialog').getByRole('button', { name: 'Delete', exact: true }).click();
386+
}
387+
388+
test('switching connections loads the other connection without asking', async ({ page }) => {
389+
const [firstKey, secondKey] = await openEditWithTwoConnections(page);
390+
await expect(page.getByLabel('Access key')).toHaveValue(firstKey);
391+
const firstUrl = page.url();
392+
393+
await sidebarConnection(page, 1).click();
394+
395+
await expect(page).not.toHaveURL(firstUrl);
396+
await expect(page.getByLabel('Access key')).toHaveValue(secondKey);
397+
await expect(page.getByRole('dialog')).toHaveCount(0);
398+
399+
// The switch must not leave the form tainted.
400+
await page.getByRole('link', { name: '← Manage connections' }).click();
401+
await expect(page).toHaveURL('/settings/connections');
402+
await expect(page.getByRole('dialog')).toHaveCount(0);
403+
});
404+
405+
test('switching connections with unsaved changes asks first', async ({ page }) => {
406+
await openEditWithTwoConnections(page);
407+
const firstUrl = page.url();
408+
409+
await page.getByLabel('Connection name').fill('Changed name');
410+
await sidebarConnection(page, 1).click();
411+
412+
await expect(page.getByRole('heading', { name: 'Unsaved changes' })).toBeVisible();
413+
await page.getByRole('dialog').getByRole('button', { name: 'Stay on page' }).click();
414+
await expect(page).toHaveURL(firstUrl);
415+
await expect(page.getByLabel('Connection name')).toHaveValue('Changed name');
416+
});
417+
418+
test('sidebar delete with unsaved changes asks in the app before deleting', async ({ page }) => {
419+
await openEditWithTwoConnections(page);
420+
let deleteRequests = 0;
421+
let nativeDialogs = 0;
422+
page.on('request', (request) => {
423+
if (request.url().endsWith('?/deleteConnection')) deleteRequests++;
424+
});
425+
page.on('dialog', (dialog) => {
426+
nativeDialogs++;
427+
void dialog.dismiss();
428+
});
429+
430+
await page.getByLabel('Connection name').fill('Changed name');
431+
await deleteFromSidebar(page, 1);
432+
433+
await expect(page.getByRole('heading', { name: 'Unsaved changes' })).toBeVisible();
434+
await page.getByRole('button', { name: 'Stay on page' }).click();
435+
await expect(page.getByRole('heading', { name: 'Unsaved changes' })).toBeHidden();
436+
await expect(page).toHaveURL(/\/edit$/);
437+
expect(deleteRequests).toBe(0);
438+
439+
await deleteFromSidebar(page, 1);
440+
await page.getByRole('button', { name: 'Leave' }).click();
441+
442+
await expect(page).toHaveURL('/settings/connections');
443+
await expect(page.locator('table tbody tr')).toHaveCount(1);
444+
expect(deleteRequests).toBe(1);
445+
expect(nativeDialogs).toBe(0);
446+
});
447+
448+
test('sidebar delete without changes deletes without asking', async ({ page }) => {
449+
await openEditWithTwoConnections(page);
450+
451+
await deleteFromSidebar(page, 1);
452+
453+
await expect(page).toHaveURL('/settings/connections');
454+
await expect(page.getByRole('heading', { name: 'Unsaved changes' })).toHaveCount(0);
455+
await expect(page.locator('table tbody tr')).toHaveCount(1);
456+
457+
// The sidebar on the next edit page no longer lists the deleted connection.
458+
await openFirstConnectionEditPage(page);
459+
await expect(
460+
page.getByRole('list', { name: 'Saved connections' }).getByRole('listitem')
461+
).toHaveCount(1);
462+
});
463+
464+
test('a failed sidebar delete shows the error and keeps the unsaved changes', async ({
465+
page
466+
}) => {
467+
await openEditWithTwoConnections(page);
468+
// An invalid ID makes the action fail.
469+
await page.route(/\?\/deleteConnection$/, (route) =>
470+
route.continue({ postData: 'connectionId=not-a-uuid' })
471+
);
472+
473+
await page.getByLabel('Connection name').fill('Changed name');
474+
await deleteFromSidebar(page, 1);
475+
await page.getByRole('button', { name: 'Leave' }).click();
476+
477+
await expect(
478+
page.getByRole('alert').filter({ hasText: 'Connection not found.' })
479+
).toBeVisible();
480+
await expect(page).toHaveURL(/\/edit$/);
481+
await expect(page.getByLabel('Connection name')).toHaveValue('Changed name');
482+
483+
// The changes are still guarded.
484+
await page.getByRole('link', { name: '← Manage connections' }).click();
485+
await expect(page.getByRole('heading', { name: 'Unsaved changes' })).toBeVisible();
486+
});
487+
});

‎e2e/storage/helpers.ts‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ async function clearSavedConnections(page: Page) {
5353
.catch(() => false)
5454
)
5555
break;
56+
const before = await items.count();
5657
// Click the last button in the first list item. On desktop this is the
5758
// "more options" button (opacity-0, so use force). On mobile it's the
5859
// X delete button which submits the form directly.
@@ -65,9 +66,15 @@ async function clearSavedConnections(page: Page) {
6566
// Confirm in the modal
6667
await page.getByRole('button', { name: 'Delete', exact: true }).click();
6768
}
68-
// The form action POSTs and the server redirects back to /storage;
69-
// wait for the page to re-hydrate before checking the list again.
70-
await waitForHydration(page);
69+
// The delete is sent without a page reload: wait until the list has changed. The last
70+
// delete empties it (desktop shows a placeholder, mobile hides the list).
71+
await expect(async () => {
72+
const changed =
73+
!(await savedList.isVisible()) ||
74+
(await savedList.getByText('No saved connections yet').isVisible()) ||
75+
(await items.count()) < before;
76+
expect(changed).toBe(true);
77+
}).toPass();
7178
}
7279
}
7380

@@ -103,14 +110,18 @@ export async function openConnectForm(page: Page, { clearSaved = true } = {}) {
103110
}
104111
}
105112

106-
export async function connectToStorage(page: Page, credentials: GarageCredentials) {
107-
await openConnectForm(page);
113+
export async function connectToStorage(
114+
page: Page,
115+
credentials: GarageCredentials,
116+
{ clearSaved = true } = {}
117+
) {
118+
await openConnectForm(page, { clearSaved });
108119
const url = new URL(credentials.endpoint);
109120
const host = url.hostname;
110121
const port = url.port;
111122
const useTls = url.protocol === 'https:';
112123

113-
await page.getByLabel('Host').fill(host);
124+
await page.getByLabel('Host', { exact: true }).fill(host);
114125
if (port) {
115126
await page.getByLabel('Port').fill(port);
116127
}

‎messages/de.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,7 @@
167167
"storage_connect_error_network": "Speicher-Backend nicht erreichbar — Host und Port prüfen.",
168168
"storage_connect_error_hdfs": "HDFS-Verbindungen werden noch nicht unterstützt.",
169169
"storage_connect_save_failed": "Die Verbindung konnte nicht gespeichert werden. Bitte erneut versuchen.",
170+
"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.",
170171
"storage_connect_host_required": "Host ist erforderlich",
171172
"storage_connect_host_invalid": "Host muss ein gültiger Hostname oder eine gültige IP-Adresse sein",
172173
"storage_connect_port_invalid": "Port muss eine ganze Zahl zwischen 1 und 65535 sein",

‎messages/en.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,7 @@
167167
"storage_connect_error_network": "Could not reach the storage backend — check the host and port.",
168168
"storage_connect_error_hdfs": "HDFS connections are not yet supported.",
169169
"storage_connect_save_failed": "Could not save the connection. Please try again.",
170+
"storage_connect_activate_failed": "The connection was saved but could not be activated. Reload the page and select it from the saved connections.",
170171
"storage_connect_host_required": "Host is required",
171172
"storage_connect_host_invalid": "Host must be a valid hostname or IP address",
172173
"storage_connect_port_invalid": "Port must be a whole number between 1 and 65535",

‎src/lib/components/storage/sidebar/StorageConnectionSidebar.svelte‎

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@
66
import TooltipTrigger from '$lib/components/TooltipTrigger.svelte';
77
import { createResizablePanel } from './resizable-panel.svelte.js';
88
import ResizeHandle from './ResizeHandle.svelte';
9+
import { enhance } from '$app/forms';
910
import { resolve } from '$app/paths';
11+
import type { SubmitFunction } from '@sveltejs/kit';
1012
import * as m from '$lib/paraglide/messages.js';
1113
import type { ConnectionMetadata } from '$lib/storage/connection-types.js';
1214
import { connectionLabel } from '$lib/storage/display-helpers.js';
@@ -25,9 +27,16 @@
2527
* connection edits rather than activating them.
2628
*/
2729
onselect?: (conn: ConnectionMetadata) => void;
30+
/**
31+
* Called before a delete is sent. Resolving `false` cancels it. Edit pages use this to ask
32+
* about unsaved changes before the connection is deleted, not after.
33+
*/
34+
onbeforedelete?: () => Promise<boolean>;
35+
/** Called once the server has answered a delete; `deleted` is false when it failed. */
36+
ondeletesettled?: (connectionId: string, deleted: boolean) => void;
2837
}
2938
30-
let { connections, activeId, onselect }: Props = $props();
39+
let { connections, activeId, onselect, onbeforedelete, ondeletesettled }: Props = $props();
3140
3241
const resize = createResizablePanel({
3342
storageKey: 'storage_connect_sidebar_width',
@@ -96,6 +105,22 @@
96105
closeContextMenu();
97106
}
98107
108+
const enhanceDelete: SubmitFunction = async ({ cancel }) => {
109+
if (onbeforedelete && !(await onbeforedelete())) {
110+
cancel();
111+
cancelDelete();
112+
return;
113+
}
114+
return async ({ formData, result, update }) => {
115+
cancelDelete();
116+
ondeletesettled?.(
117+
String(formData.get('connectionId')),
118+
result.type === 'redirect' || result.type === 'success'
119+
);
120+
await update();
121+
};
122+
};
123+
99124
const deleteConn = $derived.by<SavedConnection | null>(() => {
100125
if (!menuConn) return null;
101126
const ep = menuConn.endpoint ?? '';
@@ -111,7 +136,13 @@
111136

112137
<!-- Context menu (fixed, rendered outside the layout flow) -->
113138
{#if menuConn}
114-
<form method="POST" action="?/deleteConnection" bind:this={deleteFormEl} class="contents">
139+
<form
140+
method="POST"
141+
action="?/deleteConnection"
142+
bind:this={deleteFormEl}
143+
use:enhance={enhanceDelete}
144+
class="contents"
145+
>
115146
<input type="hidden" name="connectionId" value={menuConn.id} />
116147
</form>
117148
<ul
@@ -207,9 +238,7 @@
207238
<form
208239
method="POST"
209240
action="?/deleteConnection"
210-
onsubmit={() => {
211-
// Redirect handles the page update
212-
}}
241+
use:enhance={enhanceDelete}
213242
class="absolute -top-2 -right-2"
214243
>
215244
<input type="hidden" name="connectionId" value={conn.id} />

‎src/lib/storage/context.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,3 +4,17 @@ import type { TabsState } from './tabs.svelte.js';
44

55
export const [getStorageState, setStorageState] = createContext<StorageState>();
66
export const [getTabsState, setTabsState] = createContext<TabsState>();
7+
8+
/**
9+
* Lets the connection edit page ask about unsaved changes before the sidebar in its layout
10+
* deletes a connection.
11+
*/
12+
export interface ConnectionEditGuard {
13+
/** Resolves `true` when the delete may go ahead; the changes are then discarded. */
14+
confirmDiscard: (() => Promise<boolean>) | null;
15+
/** Brings back the discarded changes when the delete failed. */
16+
restoreChanges: (() => void) | null;
17+
}
18+
19+
export const [getConnectionEditGuard, setConnectionEditGuard] =
20+
createContext<ConnectionEditGuard>();

‎src/routes/(app)/settings/connections/[id]/+layout.svelte‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,21 +5,39 @@
55
import StorageConnectionSidebar from '$lib/components/storage/sidebar/StorageConnectionSidebar.svelte';
66
import type { ConnectionMetadata } from '$lib/storage/connection-types.js';
77
import { connectionStore } from '$lib/storage/connection-store.svelte.js';
8+
import { setConnectionEditGuard, type ConnectionEditGuard } from '$lib/storage/context.js';
89
910
let { children } = $props();
1011
1112
const connectionId = $derived(page.params.id);
1213
14+
// The edit page registers its unsaved-changes check here.
15+
const editGuard: ConnectionEditGuard = { confirmDiscard: null, restoreChanges: null };
16+
setConnectionEditGuard(editGuard);
17+
1318
function handleSelect(conn: ConnectionMetadata) {
1419
goto(resolve(`/settings/connections/${conn.id}/edit`));
1520
}
21+
22+
// Only /storage fills the store, so drop deleted connections here.
23+
function handleDeleteSettled(connectionId: string, deleted: boolean) {
24+
if (deleted) {
25+
connectionStore.connections = connectionStore.connections.filter(
26+
(c) => c.id !== connectionId
27+
);
28+
} else {
29+
editGuard.restoreChanges?.();
30+
}
31+
}
1632
</script>
1733

1834
<div class="flex flex-col gap-4 md:flex-row md:items-start">
1935
<StorageConnectionSidebar
2036
connections={connectionStore.connections}
2137
activeId={connectionId}
2238
onselect={handleSelect}
39+
onbeforedelete={() => editGuard.confirmDiscard?.() ?? Promise.resolve(true)}
40+
ondeletesettled={handleDeleteSettled}
2341
/>
2442
<div class="min-w-0 flex-1">
2543
{@render children()}

0 commit comments

Comments
 (0)