From a07a63ae5c1be16d674943f4b397a232d07f740b Mon Sep 17 00:00:00 2001
From: Guilhem Lemouel <guilhemlemouel@gmail.com>
Date: Mon, 24 Aug 2026 09:39:12 +0200
Subject: [PATCH] =?UTF-8?q?fix:=20address=20review=20round=204=20=E2=80=94?=
 =?UTF-8?q?=20unreviewed=20SQL,=20premature=20finish,=20stale=20Back?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

**Setup ran hub SQL nobody had seen.** Step 3 reviews the migrations it can run
there, but the ones deferred to setup went straight to `applyOneMigration`
against whatever database the wizard was pointed at — which can be an existing
resource holding unrelated objects. Each unconfigured row now carries a
disclosure showing exactly what will run, before "Set up" runs it.

**Finish was live while the setup decision was still outstanding.** For a project
with migrations but no resources, `execution.done` exposed the button while
`listDataTables` was still in flight and `setupNeeded` was still false — clicking
in that window left for the workspace and skipped a step the answer, a moment
later, said was needed. It now reads "Checking…" and is disabled until the check
settles.

**A reload on step 4 turned Back into a re-import.** `resume` only carries the
page's in-memory execution, so after a reload Back mounted a fresh step 3
offering Import over a bundle already in — and on a new workspace, a create that
now fails because the finished run cleared its parking. Back exists only while
the page still holds the run, which excludes exactly that case.

**`validateWorkspaceId` over-rejected a fork named `global`.** It reaches the
backend as `wm-fork-global`, which is accepted; only the effective id is checked
now, so a plain `global` is still refused. Covered by a test.

**An abandoned run left the migrate row spinning.** It is appended once the
review settles and set running by `onMigrationsStart`; stopping before its loop
left it on `running` forever, reading as work still in progress on a run that
had stopped.

Also moves the `run()` contract back onto `run()`, and gives `ImportSetupRow` an
optional `extra` snippet for detail that does not fit on one line.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 .../lib/components/ImportProjectStep.svelte   | 16 ++++++-
 .../src/lib/components/ImportSetupRow.svelte  | 43 +++++++++++--------
 .../src/lib/components/ImportSetupStep.svelte | 21 +++++++++
 .../src/lib/importWizard/execution.svelte.ts  | 20 ++++++---
 frontend/src/lib/utils/workspaceId.test.ts    |  4 ++
 frontend/src/lib/utils/workspaceId.ts         |  5 ++-
 .../projects/import/+page@(root).svelte       | 18 +++++++-
 7 files changed, 97 insertions(+), 30 deletions(-)

diff --git a/frontend/src/lib/components/ImportProjectStep.svelte b/frontend/src/lib/components/ImportProjectStep.svelte
index 6d61826886..c8b02621e9 100644
--- a/frontend/src/lib/components/ImportProjectStep.svelte
+++ b/frontend/src/lib/components/ImportProjectStep.svelte
@@ -30,6 +30,8 @@
 		onFinish: () => void
 		/** True once the run reveals data tables the destination has yet to configure. */
 		setupPending?: boolean
+		/** The page has not settled whether a setup step follows. Finishing now would skip it. */
+		setupUndecided?: boolean
 		/** Hands the run to the page, which needs the export's data tables to know
 		 * whether a setup step follows this one. */
 		onExecution?: (execution: ImportExecution | undefined) => void
@@ -50,6 +52,7 @@
 		onFinish,
 		onBack,
 		setupPending = false,
+		setupUndecided = false,
 		onExecution,
 		resume
 	}: Props = $props()
@@ -402,8 +405,17 @@
 			{/if}
 
 			{#if execution?.done}
-				<Button variant="accent" unifiedSize="sm" onClick={onFinish}>
-					{setupPending ? 'Continue →' : 'Finish setup →'}
+				<!-- Disabled while the page is still deciding whether a setup step follows:
+				     finishing in that window leaves for the workspace and skips a step that
+				     the answer, a moment later, says was needed. -->
+				<Button
+					variant="accent"
+					unifiedSize="sm"
+					disabled={setupUndecided}
+					startIcon={setupUndecided ? { icon: Loader2 } : undefined}
+					onClick={onFinish}
+				>
+					{setupUndecided ? 'Checking…' : setupPending ? 'Continue →' : 'Finish setup →'}
 				</Button>
 			{:else}
 				<Button
diff --git a/frontend/src/lib/components/ImportSetupRow.svelte b/frontend/src/lib/components/ImportSetupRow.svelte
index 3febf6fc88..11f95b333c 100644
--- a/frontend/src/lib/components/ImportSetupRow.svelte
+++ b/frontend/src/lib/components/ImportSetupRow.svelte
@@ -8,36 +8,41 @@
 		title: import('svelte').Snippet
 		detail?: import('svelte').Snippet
 		action: import('svelte').Snippet
+		/** Rendered full-width under the row, for detail that does not fit on one line. */
+		extra?: import('svelte').Snippet
 		/** Plays the confirmation flash over the action once. */
 		flash?: boolean
 	}
 
-	let { icon, title, detail, action, flash = false }: Props = $props()
+	let { icon, title, detail, action, extra, flash = false }: Props = $props()
 </script>
 
 <!-- One row for both lists on the setup step. Data tables and credentials are the same
      thing to the reader — something the import could not configure, with an action that
      configures it — so they get the same icon size, spacing and text block. -->
-<li class="flex items-center gap-3 rounded-md border border-border-light px-3 py-2 text-xs">
-	<div class="shrink-0">{@render icon()}</div>
-	<!-- No gap and no leading override: `text-xs` already carries `leading-4`, and the two
+<li class="flex flex-col rounded-md border border-border-light px-3 py-2 text-xs">
+	<div class="flex items-center gap-3">
+		<div class="shrink-0">{@render icon()}</div>
+		<!-- No gap and no leading override: `text-xs` already carries `leading-4`, and the two
 	     lines are one block of text, not two stacked items. -->
-	<div class="flex min-w-0 flex-1 flex-col">
-		{@render title()}
-		{@render detail?.()}
-	</div>
-	<!-- The confirmation flash is the one from SaveButton: the work itself happens
+		<div class="flex min-w-0 flex-1 flex-col">
+			{@render title()}
+			{@render detail?.()}
+		</div>
+		<!-- The confirmation flash is the one from SaveButton: the work itself happens
 	     elsewhere — a drawer, a wizard — so only the overlay is reused here. The button
 	     stays live underneath either way; being configured is a state, not a dead end. -->
-	<div class="relative shrink-0 overflow-hidden rounded-md">
-		{@render action()}
-		{#if flash}
-			<div
-				class="absolute inset-0 flex items-center justify-center rounded-md bg-green-200 dark:bg-green-800"
-				transition:fly={{ y: -10, duration: 300 }}
-			>
-				<CheckCircle2 class="h-5 w-5 text-green-700 dark:text-green-300" />
-			</div>
-		{/if}
+		<div class="relative shrink-0 overflow-hidden rounded-md">
+			{@render action()}
+			{#if flash}
+				<div
+					class="absolute inset-0 flex items-center justify-center rounded-md bg-green-200 dark:bg-green-800"
+					transition:fly={{ y: -10, duration: 300 }}
+				>
+					<CheckCircle2 class="h-5 w-5 text-green-700 dark:text-green-300" />
+				</div>
+			{/if}
+		</div>
 	</div>
+	{@render extra?.()}
 </li>
diff --git a/frontend/src/lib/components/ImportSetupStep.svelte b/frontend/src/lib/components/ImportSetupStep.svelte
index 548fad5875..f51b71cf61 100644
--- a/frontend/src/lib/components/ImportSetupStep.svelte
+++ b/frontend/src/lib/components/ImportSetupStep.svelte
@@ -386,6 +386,10 @@
 		{/if}
 		<ul class="flex flex-col gap-1.5">
 			{#each rows as row (row.name)}
+				{@const sql = row.migrations
+					.map((m) => m.sql)
+					.filter(Boolean)
+					.join('\n\n')}
 				<ImportSetupRow flash={row.justSaved}>
 					{#snippet icon()}
 						{#if row.status === 'done'}
@@ -414,6 +418,23 @@
 							{/if}
 						</span>
 					{/snippet}
+					{#snippet extra()}
+						<!-- The SQL, before anything runs it. Step 3 reviews the migrations it can
+						     run there; the ones deferred to here were never shown, and "Set up"
+						     executes them against whatever database the wizard is pointed at —
+						     which can be one that already holds unrelated objects. -->
+						{#if sql && row.status !== 'done'}
+							<details class="mt-1.5">
+								<summary class="cursor-pointer text-2xs text-secondary hover:text-primary">
+									Show the SQL this will run
+								</summary>
+								<pre
+									class="mt-1.5 max-h-52 overflow-auto whitespace-pre-wrap rounded border border-border-light bg-surface-secondary p-2 font-mono text-2xs text-secondary"
+									>{sql}</pre
+								>
+							</details>
+						{/if}
+					{/snippet}
 					{#snippet action()}
 						<!-- The wizard owns creating a data table: picking or provisioning the
 						     database, writing the config, and reporting the connection. This step
diff --git a/frontend/src/lib/importWizard/execution.svelte.ts b/frontend/src/lib/importWizard/execution.svelte.ts
index 7139009a7e..2017c08b27 100644
--- a/frontend/src/lib/importWizard/execution.svelte.ts
+++ b/frontend/src/lib/importWizard/execution.svelte.ts
@@ -211,13 +211,6 @@ export class ImportExecution {
 		this.tasks = this.tasks.map((t) => (t.key === key ? { ...t, status, detail } : t))
 	}
 
-	/**
-	 * Runs every task that has not already succeeded. Safe to call again after a
-	 * failure: a created workspace and a fetched export are reused rather than
-	 * repeated. The granularity is the task, not the item — a retry re-runs
-	 * `installProject` over the whole bundle, which is idempotent per item but does
-	 * not skip the ones that already landed.
-	 */
 	/**
 	 * Set when the user confirms leaving mid-run. Nothing here can abort a request already
 	 * in flight — `installProject` takes no signal — so this stops the run at the next phase
@@ -234,6 +227,13 @@ export class ImportExecution {
 		this.#abandoned = true
 	}
 
+	/**
+	 * Runs every task that has not already succeeded. Safe to call again after a
+	 * failure: a created workspace and a fetched export are reused rather than
+	 * repeated. The granularity is the task, not the item — a retry re-runs
+	 * `installProject` over the whole bundle, which is idempotent per item but does
+	 * not skip the ones that already landed.
+	 */
 	async run(): Promise<void> {
 		if (this.running) return
 		this.#abandoned = false
@@ -396,6 +396,12 @@ export class ImportExecution {
 		if (this.#abandoned) {
 			const landed = this.itemResults.length
 			this.#set('import', 'failed', `stopped after ${landed} item${landed === 1 ? '' : 's'}`)
+			// The migrate row is appended once the review settles and set running by
+			// `onMigrationsStart`. Stopping before its loop leaves it spinning forever, which
+			// reads as work still in progress on a run that has stopped.
+			if (this.tasks.some((t) => t.key === 'migrate' && t.status === 'running')) {
+				this.#set('migrate', 'pending')
+			}
 			this.error = 'Import stopped. Retry to import what is left.'
 			return
 		}
diff --git a/frontend/src/lib/utils/workspaceId.test.ts b/frontend/src/lib/utils/workspaceId.test.ts
index bcbeb1b780..8972498138 100644
--- a/frontend/src/lib/utils/workspaceId.test.ts
+++ b/frontend/src/lib/utils/workspaceId.test.ts
@@ -81,6 +81,10 @@ describe('validateWorkspaceId — the reserved id', () => {
 		expect(validateWorkspaceId('wm-fork-x', 'global')).toMatch(/not allowed/i)
 	})
 
+	it('allows a fork named `global`, which reaches the backend as `wm-fork-global`', () => {
+		expect(validateWorkspaceId('global', 'wm-fork-global')).toBeUndefined()
+	})
+
 	it('still accepts ids that merely contain it', () => {
 		expect(validateWorkspaceId('global-ops')).toBeUndefined()
 		expect(validateWorkspaceId('my-global')).toBeUndefined()
diff --git a/frontend/src/lib/utils/workspaceId.ts b/frontend/src/lib/utils/workspaceId.ts
index b1bcbd9e1b..492a00d391 100644
--- a/frontend/src/lib/utils/workspaceId.ts
+++ b/frontend/src/lib/utils/workspaceId.ts
@@ -29,7 +29,10 @@ export function validateWorkspaceId(id: string, effectiveId: string = id): strin
 	}
 	// `check_w_id_conflict` refuses it outright, and `existsWorkspace` reports it free —
 	// so without this the wizard walks the user to the last step before the create fails.
-	if (id === RESERVED_WORKSPACE_ID || effectiveId === RESERVED_WORKSPACE_ID) {
+	// Only the effective id, which is what the backend receives: it defaults to the raw one,
+	// so a plain `global` is still caught, while a fork named `global` — submitted as
+	// `wm-fork-global`, which the backend accepts — is not.
+	if (effectiveId === RESERVED_WORKSPACE_ID) {
 		return `'${RESERVED_WORKSPACE_ID}' is not allowed as a workspace ID`
 	}
 	if (effectiveId.length > WORKSPACE_ID_MAX_LENGTH) {
diff --git a/frontend/src/routes/(root)/(logged)/projects/import/+page@(root).svelte b/frontend/src/routes/(root)/(logged)/projects/import/+page@(root).svelte
index 99c0ab420d..0486e5259c 100644
--- a/frontend/src/routes/(root)/(logged)/projects/import/+page@(root).svelte
+++ b/frontend/src/routes/(root)/(logged)/projects/import/+page@(root).svelte
@@ -249,11 +249,16 @@
 	// whole wizard until the import finishes — which is exactly when it is first read.
 	let execution = $state<ImportExecution | undefined>(undefined)
 	let setupNeeded = $state(false)
+	// True while the answer is still being fetched. Without it the run reads as finished
+	// with no fourth step, and Finish leaves for the workspace before the check comes back
+	// and discovers a data table that is missing.
+	let setupUndecided = $state(false)
 	$effect(() => {
 		const names = execution?.datatableNames ?? []
 		const workspace = planWorkspaceId(plan)
 		if (!execution?.done || !workspace) {
 			setupNeeded = false
+			setupUndecided = false
 			return
 		}
 		// Every resource the project ships arrives as an empty stub, so any project with
@@ -261,13 +266,16 @@
 		// what is genuinely outstanding, which is what makes a re-import quiet.
 		if (execution.resourceCount > 0) {
 			setupNeeded = true
+			setupUndecided = false
 			return
 		}
 		if (names.length === 0) {
 			setupNeeded = false
+			setupUndecided = false
 			return
 		}
 		let cancelled = false
+		setupUndecided = true
 		void WorkspaceService.listDataTables({ workspace })
 			.then((tables) => {
 				if (cancelled) return
@@ -278,6 +286,9 @@
 				// Can't tell — don't invent a step the user then cannot complete.
 				if (!cancelled) setupNeeded = false
 			})
+			.finally(() => {
+				if (!cancelled) setupUndecided = false
+			})
 		return () => (cancelled = true)
 	})
 </script>
@@ -500,6 +511,7 @@
 				{plan}
 				{project}
 				setupPending={setupNeeded}
+				{setupUndecided}
 				onFolderChange={(folder) => go({ folder }, 3, { replace: true })}
 				onFinish={() => (setupNeeded ? go({}, 4) : finish())}
 				onBack={() => go({}, 2)}
@@ -513,7 +525,11 @@
 				folder={plan.folder}
 				onSkip={finish}
 				onFinish={finish}
-				onBack={() => go({}, 3)}
+				onBack={// Only while this page still holds the run. After a reload it does not, and a
+				// step 3 with no run offers Import again — over a bundle that is already in,
+				// and on a new workspace over a create that would now fail, because the
+				// finished run cleared its parking.
+				execution ? () => go({}, 3) : undefined}
 			/>
 		{/if}
 	</CenteredModal>