From 0b33bb67c618cac31e4aba87bac640ae2f4793ee Mon Sep 17 00:00:00 2001
From: Guilhem Lemouel <guilhemlemouel@gmail.com>
Date: Tue, 18 Aug 2026 10:31:21 +0200
Subject: [PATCH] fix(frontend): confine the modal restyle to the wizard, and
 trim the comments

The wider side padding and lighter dialog heading were changing all 17 Modal2
dialogs to suit this one flow. They move behind an opt-in `formStyling`, taken
by the three dialogs this branch owns; every other Modal2 renders as it did.

Also drops two comments that cited a design approval rather than a constraint,
and shortens the blocks that had grown past the four lines AGENTS.md asks for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 .../lib/components/common/modal/Modal2.svelte |  9 +++-
 .../AddDataTableWizard.svelte                 | 28 ++++------
 .../CustomInstanceDbWizardModal.svelte        |  1 +
 .../SupabaseConnectionMode.svelte             |  3 +-
 .../SupabaseProjectStep.svelte                |  6 +--
 .../SupabaseResourceConnect.svelte            |  1 +
 .../workspaceSettings/addDataTableModel.ts    | 52 ++++++-------------
 .../workspaceSettings/datatableProbe.ts       |  6 +--
 .../workspaceSettings/supabaseProvisioning.ts | 10 ++--
 .../src/lib/utils/postgresConnectionString.ts | 12 ++---
 10 files changed, 51 insertions(+), 77 deletions(-)

diff --git a/frontend/src/lib/components/common/modal/Modal2.svelte b/frontend/src/lib/components/common/modal/Modal2.svelte
index 367512b118..f33b1dec04 100644
--- a/frontend/src/lib/components/common/modal/Modal2.svelte
+++ b/frontend/src/lib/components/common/modal/Modal2.svelte
@@ -26,6 +26,9 @@
 		 * and clicks "outside" the child would otherwise propagate
 		 * here and close the underlying modal. */
 		closeOnOutsideClick?: boolean
+		/** Wider side padding and a lighter title, for a dialog whose body is a form rather
+		 *  than a list. Opt-in: every other Modal2 keeps the padding and heading it had. */
+		formStyling?: boolean
 		headerLeft?: import('svelte').Snippet
 		headerRight?: import('svelte').Snippet
 		children?: import('svelte').Snippet
@@ -43,6 +46,7 @@
 		fixedHeight = 'md',
 		contentClasses = '',
 		closeOnOutsideClick = true,
+		formStyling = false,
 		headerLeft,
 		headerRight,
 		children
@@ -111,7 +115,8 @@
 						heightMap[fixedHeight] ? `height: ${heightMap[fixedHeight]}; ` : ''
 					}${css?.popup?.style || ''}`}
 					class={twMerge(
-						'max-h-screen-80 max-w-screen-80 rounded-lg relative bg-surface py-4 px-6',
+						'max-h-screen-80 max-w-screen-80 rounded-lg relative bg-surface',
+						formStyling ? 'py-4 px-6' : 'p-4',
 						css?.popup?.class,
 						'wm-modal-form-popup'
 					)}
@@ -122,7 +127,7 @@
 					<List gap="md">
 						<div class="flex w-full">
 							<List horizontal justify="between">
-								<h3 class="font-semibold">{title}</h3>
+								<h3 class={formStyling ? 'font-semibold' : undefined}>{title}</h3>
 								<div class="grow w-min-0">
 									<List horizontal justify="between">
 										<div class="min-w-0 grow">
diff --git a/frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte b/frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte
index 410ef41f21..3783141849 100644
--- a/frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte
+++ b/frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte
@@ -468,13 +468,11 @@
 	}
 
 	/**
-	 * Proves the typed connection works before the wizard writes anything. The Supabase branch
-	 * has to read the pooler first: which host a project answers on is assigned by Supabase,
-	 * so the value under test has to be the value that will be saved.
+	 * Proves the connection before anything is written. Supabase reads the pooler first: the
+	 * value under test has to be the value that will be saved.
 	 */
-	// The fields stay editable while a check is out, and editing clears the verdict. Without a
-	// token the older answer lands afterwards and marks the edited connection as tested, so
-	// Continue unlocks for something nobody proved.
+	// Fields stay editable while a check is out. Without a token the older answer lands after
+	// an edit and unlocks Continue for a connection nobody proved.
 	let probeToken = 0
 	async function probe() {
 		const token = ++probeToken
@@ -539,12 +537,9 @@
 	/** The instance database this session asked for, which is registered even when it failed. */
 	let claimedInstanceDb = $state<string | undefined>(undefined)
 	/**
-	 * The name this run has already claimed. `writeRow` merges into whatever the server holds
-	 * under the name, so a name that is free in the table on screen but taken on the server --
-	 * an unsaved rename here, or another admin since the page loaded -- would repoint someone
-	 * else's data table at this database. Checked against the server before the run starts, then
-	 * remembered only if the run got as far as writing the row, so Try again can overwrite what
-	 * it wrote itself.
+	 * What this run created and may write over again. `writeRow` merges into whatever the
+	 * server holds under the name, so a name free in the table on screen but taken on the
+	 * server would repoint someone else's data table at this database.
 	 */
 	let claims = $state<Claims>(noClaims)
 	/** The path whose secret and resource this run holds, for the gates that ask by path. */
@@ -694,11 +689,9 @@
 	}
 
 	/**
-	 * Whether closing would throw away work. Until Finish the loss is only what was typed --
-	 * but that includes a pasted database password and a project about to be created, and a
-	 * backdrop click is easy to do by accident. A failed run counts too: its inputs are still
-	 * editable and it may have left something behind, which is the case most worth confirming.
-	 * A run cannot be closed while it is going, and one that succeeded has nothing left to lose.
+	 * Whether closing would throw away work. A failed run counts: its inputs are still editable
+	 * and it may have left something behind. A run in flight cannot be closed at all, and one
+	 * that succeeded has nothing left to lose.
 	 */
 	function hasUnfinishedIntent(): boolean {
 		return wiz.provider !== undefined && !run.running && !run.result?.ok
@@ -828,6 +821,7 @@
 		}
 	}
 	target="#content"
+	formStyling
 	title="Add a data table"
 	contentClasses="flex flex-col"
 	fixedWidth="md"
diff --git a/frontend/src/lib/components/workspaceSettings/CustomInstanceDbWizardModal.svelte b/frontend/src/lib/components/workspaceSettings/CustomInstanceDbWizardModal.svelte
index 0327f29f42..8246118c49 100644
--- a/frontend/src/lib/components/workspaceSettings/CustomInstanceDbWizardModal.svelte
+++ b/frontend/src/lib/components/workspaceSettings/CustomInstanceDbWizardModal.svelte
@@ -46,6 +46,7 @@
 <Modal2
 	bind:isOpen={() => !!opened, (v) => !v && !preventClose && (opened = undefined)}
 	target="#content"
+	formStyling
 	title={'Custom Instance Database Setup'}
 	contentClasses="flex flex-col"
 	fixedWidth="md"
diff --git a/frontend/src/lib/components/workspaceSettings/SupabaseConnectionMode.svelte b/frontend/src/lib/components/workspaceSettings/SupabaseConnectionMode.svelte
index 5ec944526e..6f2523a729 100644
--- a/frontend/src/lib/components/workspaceSettings/SupabaseConnectionMode.svelte
+++ b/frontend/src/lib/components/workspaceSettings/SupabaseConnectionMode.svelte
@@ -42,8 +42,7 @@
 	</button>
 	{#if open}
 		<div class="flex flex-col gap-1.5 mt-2">
-			<!-- Not `RadioCard`: these carry no radio dot and use the accent selected style, which
-			is the design that was approved for this panel. -->
+			<!-- Not `RadioCard`: no radio dot, and selection reads through the accent surface. -->
 			{#each OPTIONS as option (option.value)}
 				{@const selected = mode === option.value}
 				<button
diff --git a/frontend/src/lib/components/workspaceSettings/SupabaseProjectStep.svelte b/frontend/src/lib/components/workspaceSettings/SupabaseProjectStep.svelte
index ea1987a361..7979c5cb6e 100644
--- a/frontend/src/lib/components/workspaceSettings/SupabaseProjectStep.svelte
+++ b/frontend/src/lib/components/workspaceSettings/SupabaseProjectStep.svelte
@@ -150,9 +150,9 @@
 		{#each projects ?? [] as p (projectRef(p))}
 			{@const selected = intent.mode === 'existing' && isSelected(intent.project, p)}
 			<!-- Not `RadioCard`: the selected card opens to hold a password field, and these carry
-			a project icon and no radio dot -- the design that was approved for this step.
-			shrink-0 or the flex column squeezes the cards to fit instead of letting the
-			list scroll, and the selected one loses its password field to the clip. -->
+			a project icon and no radio dot. shrink-0 or the flex column squeezes the cards to
+			fit instead of letting the list scroll, and the selected one loses its password
+			field to the clip. -->
 			<div
 				class="shrink-0 border rounded-md overflow-hidden transition-colors {selected
 					? 'border-border-selected/50 bg-surface-accent-selected'
diff --git a/frontend/src/lib/components/workspaceSettings/SupabaseResourceConnect.svelte b/frontend/src/lib/components/workspaceSettings/SupabaseResourceConnect.svelte
index f8fd2b0215..f9f4586503 100644
--- a/frontend/src/lib/components/workspaceSettings/SupabaseResourceConnect.svelte
+++ b/frontend/src/lib/components/workspaceSettings/SupabaseResourceConnect.svelte
@@ -93,6 +93,7 @@
 <Modal2
 	bind:isOpen={open}
 	target="#content"
+	formStyling
 	title="Connect Supabase"
 	contentClasses="flex flex-col"
 	fixedWidth="md"
diff --git a/frontend/src/lib/components/workspaceSettings/addDataTableModel.ts b/frontend/src/lib/components/workspaceSettings/addDataTableModel.ts
index dac086151f..f144cb81ae 100644
--- a/frontend/src/lib/components/workspaceSettings/addDataTableModel.ts
+++ b/frontend/src/lib/components/workspaceSettings/addDataTableModel.ts
@@ -200,13 +200,9 @@ const RESERVED_DB_NAMES = ['template0', 'template1', 'postgres']
 const VALID_DB_NAME = /^[a-zA-Z][a-zA-Z0-9_-]*$/
 
 /**
- * Why `setup_custom_instance_db` would refuse this name, checked as it is typed rather than
- * at the end of a run that creates a billed project first.
- *
- * Deliberately not exhaustive: the instance may hold databases Windmill did not create, and
- * only the server knows its own database's name. The backend stays the authority; this is
- * here to catch the cases the browser already has the answer to. Empty is not an error --
- * the step is simply incomplete, and shouting at an untouched field is noise.
+ * Why `setup_custom_instance_db` would refuse this name, checked as it is typed. Deliberately
+ * not exhaustive -- the backend stays the authority, this only catches what the browser
+ * already knows. Empty is incomplete rather than wrong.
  */
 export function instanceDbNameError(name: string, existing: Iterable<string>): string | undefined {
 	const trimmed = name.trim()
@@ -224,12 +220,9 @@ export function instanceDbNameError(name: string, existing: Iterable<string>): s
 const VALID_DATATABLE_NAME = /^[a-zA-Z0-9][a-zA-Z0-9_\-.]*$/
 
 /**
- * Why `edit_datatable_config` would refuse this name. Checked as it is typed because the
- * write is the *last* step of the run: by the time the backend rejects it, a Supabase
- * project may have been billed and the resource and secret already written, and the name is
- * no longer editable without closing the wizard.
- *
- * `existing` are the names already in the workspace.
+ * Why `edit_datatable_config` would refuse this name, checked as it is typed because the write
+ * is the *last* step of the run: by the time the backend rejects it a Supabase project may
+ * have been billed. `existing` are the names already in the workspace.
  */
 export function datatableNameError(name: string, existing: Iterable<string>): string | undefined {
 	const trimmed = name.trim()
@@ -449,13 +442,9 @@ async function writeRow(
 }
 
 /**
- * Take back a row this run wrote. Reports whether it went, since a failure to undo leaves the
- * data table in the config and the caller has to keep saying so.
- */
-/**
- * `removed` — the row this run wrote is gone. `kept` — it is still there, the undo could not
- * reach the server. `foreign` — the name now points somewhere this run never wrote, so there
- * is nothing of ours to take back and the name is no longer ours to claim.
+ * `removed` — the row this run wrote is gone. `kept` — the undo could not reach the server, so
+ * it is still there and the caller has to keep saying so. `foreign` — the name now points
+ * somewhere this run never wrote, so there is nothing of ours to take back.
  */
 type Rollback = 'removed' | 'kept' | 'foreign'
 
@@ -607,14 +596,10 @@ export async function runSetup(state: WizardState, deps: RunDeps): Promise<RunRe
 	const path = resourcePathOf(state)
 	const name = state.review.name.trim()
 	/**
-	 * This run is pointed at the path where an earlier attempt stored a created project's
-	 * password. Supabase hands that password out once, and every write to the path upserts, so
-	 * the three routes back to it -- connecting the new project as an existing one, pointing the
-	 * wizard at a hand-written connection, and creating a second project -- all refuse on it.
-	 * Aim the run somewhere else and there is nothing left to protect.
+	 * An earlier attempt stored a created project's password here. Supabase hands that out once
+	 * and every write upserts, so every route back to this path refuses. Each created project
+	 * guards its own path -- checking only the latest unlocked the earlier one's password.
 	 */
-	// Every project created this session guards its own path. Checking only the latest let a
-	// second attempt at another path unlock the first one's password for overwriting.
 	const guardedHere = deps.createdProjects.find((p) => p.path === path)
 	const instanceName = state.instance.dbName?.trim() ?? ''
 
@@ -651,14 +636,11 @@ export async function runSetup(state: WizardState, deps: RunDeps): Promise<RunRe
 					}
 					project = existing
 				} else {
-					// The earlier attempt's project has to still be there for its password to be worth
-					// protecting. A name recorded because a create could not be confirmed, with no
-					// project behind it, was a false alarm -- and refusing on it would leave the
-					// session with nothing it could do.
-					// By name alone, not `inOrg`: the earlier project was created under whatever
-					// organization was selected then, and the one selected now may be a different
-					// one -- which is itself a way to arrive here. Refusing a namesake in another
-					// organization costs a rename; missing the real one costs the password.
+					// The project has to still exist for its password to be worth protecting: a name
+					// recorded from a create that could not be confirmed is a false alarm, and
+					// refusing on it leaves the session with nothing it can do. Matched by name
+					// across every organization -- a namesake costs a rename, a miss costs the
+					// password.
 					const earlier = guardedHere?.name
 					if (earlier && projects.some((p) => p.name === earlier)) {
 						return fail(createdSecretRefusal(earlier, guardedHere!.path))
diff --git a/frontend/src/lib/components/workspaceSettings/datatableProbe.ts b/frontend/src/lib/components/workspaceSettings/datatableProbe.ts
index c04ff1de50..c66aa9490d 100644
--- a/frontend/src/lib/components/workspaceSettings/datatableProbe.ts
+++ b/frontend/src/lib/components/workspaceSettings/datatableProbe.ts
@@ -35,10 +35,8 @@ type Row = {
 }
 
 /**
- * `database` is whatever a Postgres step takes: the resource value itself, or a `$res:` path
- * the worker resolves.
- *
- * Throws with the database's own message when the connection or the query fails, and after
+ * `database` is whatever a Postgres step takes: the resource value, or a `$res:` path the
+ * worker resolves. Throws with the database's own message when the query fails, and after
  * `timeout` when no worker picks the job up.
  */
 export async function probeDatatableConnection(
diff --git a/frontend/src/lib/components/workspaceSettings/supabaseProvisioning.ts b/frontend/src/lib/components/workspaceSettings/supabaseProvisioning.ts
index 88ddbcd3fe..5040b5ab38 100644
--- a/frontend/src/lib/components/workspaceSettings/supabaseProvisioning.ts
+++ b/frontend/src/lib/components/workspaceSettings/supabaseProvisioning.ts
@@ -203,12 +203,10 @@ export type SupabaseConnection = {
 }
 
 /**
- * The endpoint a project should be reached through, degrading rather than failing.
- *
- * Reading the pooler config needs the `database_pooling_config_read` scope, which an
- * instance's Supabase OAuth app may not have been granted -- and no amount of retrying
- * fixes that. A direct connection still works for anyone whose workers have IPv6, so
- * fall back to it and say so, instead of leaving the user with no way through.
+ * The endpoint a project should be reached through, degrading rather than failing. Reading the
+ * pooler config needs the `database_pooling_config_read` scope, which an instance's OAuth app
+ * may not have. A direct connection still works where the workers have IPv6, so fall back to
+ * it and say so.
  */
 export async function resolveSupabaseConnection(
 	token: string,
diff --git a/frontend/src/lib/utils/postgresConnectionString.ts b/frontend/src/lib/utils/postgresConnectionString.ts
index 5edbfdbe1e..6dedcfeed5 100644
--- a/frontend/src/lib/utils/postgresConnectionString.ts
+++ b/frontend/src/lib/utils/postgresConnectionString.ts
@@ -76,14 +76,10 @@ const REPRESENTABLE_PARAMS = ['sslmode']
 const COSMETIC_PARAMS = ['application_name']
 
 /**
- * The name of a parameter this string carries that the resource cannot honour.
- *
- * An allowlist rather than a list of known-bad names: libpq keeps adding parameters, and the
- * ones that matter most are the ones that would be missed. `sslrootcert=system` demands full
- * certificate verification, `channel_binding=require` demands SCRAM binding,
- * `target_session_attrs=read-write` decides which node is acceptable, `options=-csearch_path=…`
- * decides which schema tables land in. Dropping any of them saves a connection weaker or
- * simply other than the one that was pasted, behind a probe that reports success.
+ * The name of a parameter this string carries that the resource cannot honour. An allowlist,
+ * not a list of known-bad names: libpq keeps adding parameters, and the ones that matter are
+ * the ones that would be missed. Dropping one silently saves a connection weaker or simply
+ * other than the one pasted, behind a probe that reports success.
  */
 export function unsupportedConnectionParam(connectionString: string): string | undefined {
 	const query = connectionString.split('?').slice(1).join('?')