fix(frontend): read connection strings the way libpq does

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

backend/ee-repo-ref.txt

@@ -1 +1 @@
-71ef2cf2a5badd53d16d5cc945ab4ada1a639314
+81a16d2aaab969e3e22b128e736e833120f70105

frontend/src/lib/components/wizards/SetupChecklist.svelte

@@ -37,12 +37,20 @@
 
 	let openedDescriptions: Record<number, true> = $state({})
 
+	function toggleDescription(i: number) {
+		if (openedDescriptions[i]) delete openedDescriptions[i]
+		else openedDescriptions[i] = true
+	}
+
 	$effect(() => {
 		for (let i = 0; i < steps.length; i++) {
 			if (steps[i].status === 'failed') openedDescriptions[i] = true
 		}
 	})
 
+	const titleRowClass = (status: SetupStepStatus) =>
+		twMerge('text-xs font-medium flex justify-between items-center', titleClass[status])
+
 	const titleClass: Record<SetupStepStatus, string> = {
 		pending: 'text-hint/75',
 		running: 'text-primary',
@@ -55,17 +63,7 @@
 <div class={twMerge('flex flex-col gap-0.5', className)}>
 	{#each steps as step, i}
 		{@const descriptionOpened = openedDescriptions[i] ?? false}
-		<!-- svelte-ignore a11y_click_events_have_key_events -->
-		<!-- svelte-ignore a11y_no_static_element_interactions -->
-		<div
-			class="flex flex-col bg-surface rounded-md py-1 pr-2 cursor-pointer"
-			role=""
-			onclick={() => {
-				if (!step.description) return
-				if (descriptionOpened) delete openedDescriptions[i]
-				else openedDescriptions[i] = true
-			}}
-		>
+		<div class="flex flex-col bg-surface rounded-md py-1 pr-2">
 			<div class="flex gap-2">
 				<span class="inline-flex w-4 h-5 shrink-0 justify-center items-center">
 					{#if step.status === 'running'}
@@ -79,14 +77,15 @@
 					{/if}
 				</span>
 				<div class="flex-1 my-0.5">
-					<span
-						class={twMerge(
-							'text-xs font-medium flex justify-between items-center',
-							titleClass[step.status]
-						)}
-					>
-						{step.title}
-						{#if step.description}
+					<!-- The title is the whole interactive surface, so a step without a description
+					stays inert rather than offering a focus stop that does nothing. -->
+					{#if step.description}
+						<button
+							type="button"
+							class={twMerge(titleRowClass(step.status), 'w-full text-left cursor-pointer')}
+							onclick={() => toggleDescription(i)}
+						>
+							{step.title}
 							<ChevronDown
 								class={twMerge(
 									'text-hint transition-transform',
@@ -94,14 +93,13 @@
 								)}
 								size={14}
 							/>
-						{/if}
-					</span>
+						</button>
+					{:else}
+						<span class={titleRowClass(step.status)}>{step.title}</span>
+					{/if}
 					<ResizeTransitionWrapper vertical class="text-2xs text-secondary">
 						{#if descriptionOpened}
-							<div
-								class="whitespace-pre-wrap cursor-default mt-1.5"
-								onclick={(e) => e.stopPropagation()}
-							>
+							<div class="whitespace-pre-wrap mt-1.5">
 								{step.description}
 							</div>
 						{/if}
@@ -109,7 +107,7 @@
 				</div>
 			</div>
 			{#if step.substeps?.length}
-				<div class="ml-6" onclick={(e) => e.stopPropagation()}>
+				<div class="ml-6">
 					<Self steps={step.substeps} />
 				</div>
 			{/if}

frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte

@@ -415,6 +415,9 @@
 		// Read off one attempt against one project; the review step would otherwise warn about
 		// a limitation that no longer applies while claiming session pooling right above it.
 		poolerUnavailable = undefined
+		// Same for the failure carried back to the review step: it names inputs that have since
+		// been edited, so it would describe a run nobody can still act on.
+		lastFailure = ''
 		if (maxStep > wiz.step) maxStep = wiz.step
 	}
 
@@ -650,7 +653,10 @@
 	 * at all while it is going, and once it has a result there is nothing left to lose.
 	 */
 	function hasUnfinishedIntent(): boolean {
-		return wiz.provider !== undefined && !run.running && !run.result
+		// A failed run counts: its inputs are still editable and it is the case that can have
+		// left something behind, so leaving then is the discard most worth confirming. Only a
+		// run that succeeded has nothing left to lose.
+		return wiz.provider !== undefined && !run.running && !run.result?.ok
 	}
 
 	/** Backdrop, Escape and the close button all arrive here. */

frontend/src/lib/utils/postgresConnectionString.test.ts

@@ -36,24 +36,18 @@ describe('parsePostgresConnectionString', () => {
 		expect(parsePostgresConnectionString('')).toBeUndefined()
 	})
 
-	// Only the last `@` can be the one before the host, and passwords hold `@` often enough
-	// that splitting at the first would silently keep half of one and connect nowhere.
-	it('splits credentials at the last @', () => {
-		expect(
-			parsePostgresConnectionString('postgres://u:p@ss@db.example.com:5432/mydb')
-		).toMatchObject({ password: 'p@ss', host: 'db.example.com', port: 5432 })
-	})
-
-	// A password is free to contain a `%`, which is not the start of an escape, so nothing
-	// here is unescaped -- a credential means the characters it is written with.
-	it('keeps percent escapes as typed', () => {
-		expect(parsePostgresConnectionString('postgres://u:p%40ss@host/db')?.password).toBe('p%40ss')
+	// Verified against psql: `postgres://role:p%40ss@host/db` authenticates as `p@ss`, and an
+	// unencoded `@` puts the rest of the password in libpq's host too. Reading these any other
+	// way would make the same string mean something here that it means nowhere else.
+	it('decodes percent escapes in credentials, as libpq does', () => {
+		expect(parsePostgresConnectionString('postgres://u:p%40ss@host/db')?.password).toBe('p@ss')
+		expect(parsePostgresConnectionString('postgres://u%40corp:p@host/db')?.user).toBe('u@corp')
 	})
 })
 
 // The wizard offers the same connection as a string or as fields and switches between them
-// by composing and reparsing, so a password holding a reserved character has to survive the
-// trip: it would otherwise come back wrong rather than failing to parse.
+// by composing and reparsing. A password holding a character the URI reserves is the case
+// that breaks silently: it comes back wrong rather than failing to parse.
 describe('composePostgresConnectionString', () => {
 	it('leaves an explicit prefer out of the string, since libpq assumes it', () => {
 		const parts = { user: 'u', host: 'h', port: undefined, dbname: 'db', sslmode: 'prefer' }
@@ -66,8 +60,8 @@ describe('composePostgresConnectionString', () => {
 
 	it('round-trips through parse', () => {
 		const parts = {
-			user: 'u',
-			password: 'p@ss/w:rd%',
+			user: 'u@corp',
+			password: 'p@ss/w:rd',
 			host: 'db.example.com',
 			port: 6543,
 			dbname: 'mydb',

frontend/src/lib/utils/postgresConnectionString.ts

@@ -9,14 +9,14 @@
  * user switch, so parse and compose have to be inverses: whatever one produces,
  * the other must read back unchanged.
  *
- * Credentials are split at the *last* `@`, since that is the only one that can precede a
- * host: a password holding an unescaped `@` is common enough that reading the first one
- * would quietly put half the password in the host. Everything else is taken literally --
- * a percent escape stays the characters that were typed.
+ * libpq is the arbiter of what a connection string means, so this follows it rather than
+ * RFC 3986 where they differ: credentials are split at the *first* `@` -- an unencoded one
+ * lands in the host for libpq too -- and percent escapes in them are decoded, so `p%40ss`
+ * authenticates as `p@ss`.
  */
 
 const CONNECTION_STRING =
-	/postgres(?:ql)?:\/\/(?<user>[^:@]+)(?::(?<password>.+))?@(?<host>[^:\/?@]+)(?::(?<port>\d+))?\/(?<dbname>[^\?]+)?(?:\?.*sslmode=(?<sslmode>[^&]+))?/
+	/postgres(?:ql)?:\/\/(?<user>[^:@]+)(?::(?<password>[^@]+))?@(?<host>[^:\/?]+)(?::(?<port>\d+))?\/(?<dbname>[^\?]+)?(?:\?.*sslmode=(?<sslmode>[^&]+))?/
 
 /**
  * A database someone types into Windmill is almost never localhost, so callers ask for TLS
@@ -33,6 +33,15 @@ export type PostgresConnectionParts = {
 	sslmode?: string
 }
 
+/** A lone `%` is not an escape, and a password is free to contain one. */
+function decode(value: string): string {
+	try {
+		return decodeURIComponent(value)
+	} catch {
+		return value
+	}
+}
+
 /** Undefined when the string is not a postgres URI. */
 export function parsePostgresConnectionString(
 	connectionString: string
@@ -41,8 +50,8 @@ export function parsePostgresConnectionString(
 	if (!match?.groups) return undefined
 	const { user, password, host, port, dbname, sslmode } = match.groups
 	return {
-		user,
-		password: password || undefined,
+		user: decode(user),
+		password: password ? decode(password) : undefined,
 		host,
 		port: port ? Number(port) : undefined,
 		dbname: dbname || undefined,
@@ -55,7 +64,9 @@ export function parsePostgresConnectionString(
  * string stays the short one people recognize when nothing was overridden.
  */
 export function composePostgresConnectionString(parts: PostgresConnectionParts): string {
-	const credentials = parts.password ? `${parts.user}:${parts.password}` : parts.user
+	const credentials = parts.password
+		? `${encodeURIComponent(parts.user)}:${encodeURIComponent(parts.password)}`
+		: encodeURIComponent(parts.user)
 	const port = parts.port ? `:${parts.port}` : ''
 	const query = parts.sslmode && parts.sslmode !== 'prefer' ? `?sslmode=${parts.sslmode}` : ''
 	return `postgres://${credentials}@${parts.host}${port}/${parts.dbname ?? ''}${query}`