fix: read the destination's real state instead of inferring it

Three ways step 4 could report work that had not happened:

AddDataTableWizard wrote through $workspaceStore while ImportSetupStep used
the workspace from the URL plan. The import page is reparented out of
(logged), so nothing re-runs the layout's workspace persistence; after a
reload the store still named the workspace the user came from. "Set up"
would then create the data table there and run the migrations in the
destination. The workspace is now a prop, defaulting to the store so every
other call site is unchanged.

load() marked a row done whenever the data table name existed. The wizard
creates the table and the migrations run after it, so a table can be there
with none of the project's tables inside it — and a reload rebuilds rows
from scratch, hiding the failure. It now asks probeMigrationApplied, which
already existed for exactly this question. An undefined answer ("cannot
tell") keeps whatever the row said rather than inventing an outstanding row.

A reviewed migration could fail in step 3 while the run still reported a
clean finish: the migrate row said failed, but `error` was set only from
item failures, and `error` is what offers Retry.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xg8vXUuHCH3aRkf91sfxjx

frontend/src/lib/components/ImportSetupStep.svelte

@@ -18,6 +18,7 @@
 	import { registryCcCapableFor } from '$lib/components/oauthRegistry'
 	import { resourceTypeDisplayName } from '$lib/components/resourceTypeDisplay'
 	import { applyOneMigration } from '$lib/components/workspaceSettings/projectInstall'
+	import { probeMigrationApplied } from '$lib/importWizard/probe'
 	import {
 		retargetProjectExport,
 		type ProjectExport,
@@ -133,6 +134,32 @@
 		return `dt${n}`
 	}
 
+	/**
+	 * What a row's state actually is, asked of the destination rather than inferred.
+	 *
+	 * The data table existing is not the question — the wizard creates it and the migrations
+	 * run afterwards, so a table can be there with none of the project's tables inside it.
+	 * That gap is invisible in memory after a reload, which rebuilds every row from scratch;
+	 * reading it as "done" would let the step say "You're all set" over a project whose apps
+	 * all fail on open.
+	 *
+	 * `probeMigrationApplied` answers `undefined` when it cannot tell (unreadable schema, or
+	 * SQL it cannot resolve to table names). That is not evidence of failure, so it keeps
+	 * whatever the row already said instead of manufacturing an outstanding row nobody can
+	 * clear.
+	 */
+	async function settle(
+		ms: ProjectMigration[],
+		absent: boolean,
+		prev: Row | undefined
+	): Promise<Row['status']> {
+		if (absent) return 'unconfigured'
+		const applied = await Promise.all(ms.map((m) => probeMigrationApplied(workspace, m)))
+		if (applied.every((a) => a === true)) return 'done'
+		if (applied.some((a) => a === false)) return prev?.status === 'failed' ? 'failed' : 'unconfigured'
+		return prev?.status === 'failed' ? 'failed' : 'done'
+	}
+
 	/** Which data tables the project needs that the destination does not have yet. */
 	async function load() {
 		loading = true
@@ -152,19 +179,17 @@
 			const missing = [...new Set(enabled.map((m) => m.datatable_name))].filter(
 				(n) => !present.has(n)
 			)
-			// Rows already configured in an earlier pass keep their state; the wizard only
-			// ever adds data tables, so a name that has left `missing` is done.
 			const previous = new Map(rows.map((r) => [r.name, r]))
-			rows = [...new Set(enabled.map((m) => m.datatable_name))].map((name) => {
-				const prev = previous.get(name)
-				if (prev && !missing.includes(name)) return { ...prev, status: 'done' as const }
-				return {
-					name,
-					migrations: enabled.filter((m) => m.datatable_name === name),
-					status: (missing.includes(name) ? 'unconfigured' : 'done') as Row['status'],
-					justSaved: false
-				}
-			})
+			rows = await Promise.all(
+				[...new Set(enabled.map((m) => m.datatable_name))].map(async (name) => {
+					const ms = enabled.filter((m) => m.datatable_name === name)
+					const prev = previous.get(name)
+					const status = await settle(ms, missing.includes(name), prev)
+					return prev
+						? { ...prev, migrations: ms, status, error: status === 'done' ? undefined : prev.error }
+						: { name, migrations: ms, status, justSaved: false }
+				})
+			)
 			// Retargeted the same way the import was, so these are where the stubs actually
 			// landed. `retargetProjectExport` is a no-op when the folder is the slug, which is
 			// every new-workspace import.
@@ -597,6 +622,7 @@
 		bind:opened={wizardOpen}
 		initialName={wizardFor}
 		modalTarget="body"
+		{workspace}
 		finishAlso="run migrations"
 		onFinishAlso={() => runMigrationsFor(wizardFor ?? '')}
 		existingNames={configuredNames.map((c) => c.name)}

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

@@ -102,6 +102,16 @@
 		 * where the rest of the run does instead of starting after the dialog closes.
 		 * Throwing marks that step failed; the table itself is already made either way. */
 		onFinishAlso?: () => Promise<void>
+		/** The workspace everything here is created in and checked against.
+		 *
+		 * Defaults to `$workspaceStore`, which is right for the settings page — it is the
+		 * workspace being looked at. The import wizard is the exception: its page is
+		 * reparented out of `(logged)`, so nothing re-runs the layout's workspace
+		 * persistence, and after a reload the store still names whatever workspace the
+		 * user came from while the plan in the URL names the destination. Left ambient,
+		 * this would create the data table in one workspace and run the project's
+		 * migrations in the other. */
+		workspace?: string
 	}
 
 	let {
@@ -116,9 +126,13 @@
 		initialName,
 		modalTarget = '#content',
 		finishAlso,
-		onFinishAlso
+		onFinishAlso,
+		workspace: workspaceProp
 	}: Props = $props()
 
+	/** Every write and every check goes through this, never `$workspaceStore` directly. */
+	const targetWorkspace = $derived(workspaceProp ?? $workspaceStore ?? '')
+
 	const STEPS = ['Choose a database', 'Set it up', 'Review']
 
 	let wiz: WizardState = $state(
@@ -167,7 +181,7 @@
 		}
 		clearTimeout(variableCheck)
 		variableCheck = setTimeout(async () => {
-			const taken = await VariableService.existsVariable({ workspace: $workspaceStore!, path })
+			const taken = await VariableService.existsVariable({ workspace: targetWorkspace, path })
 			// Two checks can be in flight at once and resolve out of order. A `false` for a path
 			// nobody is on any more would clear the error guarding the one about to be written;
 			// a `true` would disable Finish over a path this run stopped caring about.
@@ -184,7 +198,7 @@
 	 * in flight when Finish is pressed.
 	 */
 	async function pathConflictMessage(path: string): Promise<string | undefined> {
-		const workspace = $workspaceStore!
+		const workspace = targetWorkspace
 		// Each namespace answers to its own claim. Holding the secret says nothing about who owns
 		// the resource beside it, so one claim must not wave the other's check through.
 		const [variable, resource] = await Promise.all([
@@ -199,14 +213,14 @@
 	let maxStep = $state(1)
 
 	function defaultProjectName(): string {
-		return `windmill-${$workspaceStore ?? 'workspace'}`
+		return `windmill-${targetWorkspace || 'workspace'}`
 	}
 
 	function defaultTableName(): string {
 		// A caller that needs a specific name wins over the usual "main, unless taken":
 		// the import wizard's migrations only apply to a table of the name they target.
 		if (initialName) return initialName
-		return existingNames.includes('main') ? `${$workspaceStore ?? 'data'}_datatable` : 'main'
+		return existingNames.includes('main') ? `${targetWorkspace || 'data'}_datatable` : 'main'
 	}
 
 	// Takes the list rather than reading it, so the fetch that loads it can seed off its own
@@ -260,7 +274,7 @@
 	)
 
 	const pgResources = resource(
-		() => (opened && wiz.provider === 'resource' ? ($workspaceStore ?? '') : ''),
+		() => (opened && wiz.provider === 'resource' ? targetWorkspace : ''),
 		async (workspace) => {
 			if (!workspace) return undefined
 			const list = await ResourceService.listResource({ workspace, resourceType: 'postgresql' })
@@ -354,7 +368,7 @@
 	)
 
 	const folderNames = resource(
-		() => (opened ? ($workspaceStore ?? '') : ''),
+		() => (opened ? targetWorkspace : ''),
 		async (workspace) => {
 			if (!workspace) return []
 			const all = await FolderService.listFolderNames({ workspace })
@@ -556,7 +570,7 @@
 				settle({ checking: false, report: undefined, error: undefined })
 				return
 			}
-			const report = await probeDatatableConnection($workspaceStore!, database)
+			const report = await probeDatatableConnection(targetWorkspace, database)
 			settle({ checking: false, report, error: undefined })
 		} catch (err: any) {
 			settle({
@@ -667,7 +681,7 @@
 		const name = wiz.review.name.trim()
 		try {
 			if (claimedName !== name) {
-				const settings = await WorkspaceService.getSettings({ workspace: $workspaceStore! })
+				const settings = await WorkspaceService.getSettings({ workspace: targetWorkspace })
 				if (settings.datatable?.datatables?.[name]) {
 					nameConflictFor = {
 						name,
@@ -712,7 +726,7 @@
 		let result: RunResult | undefined = undefined
 		try {
 			result = await runSetup(wiz, {
-				workspace: $workspaceStore!,
+				workspace: targetWorkspace,
 				supabaseToken: supaOauth.token,
 				onInstanceDbsChanged: async () => {
 					await customInstanceDbs.refetch()
@@ -1128,7 +1142,7 @@
 	{#if wiz.instance.mode === 'existing'}
 		{@const shared = (
 			customInstanceDbs.current?.[wiz.instance.dbName ?? '']?.used_by_workspaces ?? []
-		).filter((w) => w !== $workspaceStore)}
+		).filter((w) => w !== targetWorkspace)}
 		<!-- Above the list, not under it: the list scrolls, and a warning about sharing another
 		workspace's data is worthless if the user has to scroll to reach it. -->
 		{#if shared.length}
@@ -1141,7 +1155,7 @@
 		<div class="flex flex-col gap-2 overflow-y-auto flex-1 min-h-24 pr-1">
 			{#each instanceDbs as { name, db } (name)}
 				{@const selected = wiz.instance.dbName === name}
-				{@const others = (db.used_by_workspaces ?? []).filter((w) => w !== $workspaceStore)}
+				{@const others = (db.used_by_workspaces ?? []).filter((w) => w !== targetWorkspace)}
 				<button
 					class="text-left border rounded-md p-3 flex gap-3 items-start transition-colors {selected
 						? 'border-border-selected/50 bg-surface-accent-selected'

frontend/src/lib/importWizard/execution.svelte.ts

@@ -419,6 +419,7 @@ export class ImportExecution {
 		this.#set('import', failed > 0 ? 'failed' : 'done', parts.join(', '))
 
 		const migrated = this.migrationResults
+		const badMigrations = migrated.filter((r) => !r.ok).length
 		if (migrated.length) {
 			const badly = migrated.filter((r) => !r.ok)
 			this.#set(
@@ -430,7 +431,17 @@ export class ImportExecution {
 		// A partial import is finished, not broken: the items that landed are real,
 		// and the failures are listed. Only a hard stop leaves `done` false.
 		this.done = true
-		if (failed > 0) this.error = `${failed} item${failed === 1 ? '' : 's'} failed to import.`
+		// Both kinds of failure, because `error` is what offers Retry. A migration that failed
+		// against an existing data table used to leave the row saying `failed` with `error`
+		// unset — the run then presented as a clean finish, with no way to run it again.
+		const problems: string[] = []
+		if (failed > 0) problems.push(`${failed} item${failed === 1 ? '' : 's'} failed to import`)
+		if (badMigrations > 0) {
+			problems.push(
+				`${badMigrations} data table migration${badMigrations === 1 ? '' : 's'} failed`
+			)
+		}
+		if (problems.length) this.error = `${problems.join(', ')}.`
 	}
 
 	/**

frontend/src/lib/importWizard/migrationOutcome.test.ts

@@ -0,0 +1,94 @@
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+/**
+ * A run that imported every item but failed a data table migration is not a clean finish.
+ * `error` is what offers Retry, so this is about whether the user can act on the failure —
+ * the `migrate` row already says it happened.
+ */
+
+vi.mock('$lib/gen', () => ({
+	WorkspaceService: {
+		createWorkspace: vi.fn(),
+		listDataTables: vi.fn(async () => []),
+		listWorkspaces: vi.fn(async () => [])
+	},
+	UserService: {
+		whoami: vi.fn(async () => ({ username: 'u' })),
+		globalWhoami: vi.fn(async () => ({ email: 'u@example.com' }))
+	}
+}))
+vi.mock('$lib/storeUtils', () => ({ switchWorkspace: vi.fn() }))
+vi.mock('./probe', async (orig) => ({
+	...(await orig<typeof import('./probe')>()),
+	probeWorkspace: vi.fn(async () => ({ exists: false, ours: false })),
+	probeImportedPaths: vi.fn(async () => new Set<string>())
+}))
+vi.mock('$lib/user', () => ({ getUserExt: vi.fn(async () => ({ username: 'u' })) }))
+
+/** Whether the migration this run applies succeeds. The item writes always do. */
+const outcome = vi.hoisted(() => ({ migrationOk: true }))
+
+vi.mock('$lib/components/workspaceSettings/projectInstall', () => ({
+	installProject: vi.fn(async (args: any) => {
+		args.onResult({ path: 'f/calendly/one', ok: true })
+		if (args.migrations?.length) {
+			args.onMigrationsStart?.()
+			for (const m of args.migrations) {
+				args.onResult({
+					path: `data table: ${m.datatable_name}`,
+					ok: outcome.migrationOk,
+					error: outcome.migrationOk ? undefined : 'relation already exists'
+				})
+			}
+		}
+	})
+}))
+
+const EXPORT = {
+	project: { slug: 'calendly', name: 'Calendly', summary: '', readme: null },
+	scripts: [],
+	flows: [],
+	apps: [],
+	resources: [],
+	triggers: [],
+	migrations: []
+}
+vi.stubGlobal(
+	'fetch',
+	vi.fn(async () => ({ ok: true, status: 200, text: async () => JSON.stringify(EXPORT) }))
+)
+
+import { ImportExecution } from './execution.svelte'
+
+const PLAN = { slug: 'calendly', destination: { kind: 'existing' as const, workspaceId: 'ws-a' } }
+const MIGRATION = {
+	datatable_name: 'main',
+	sql: 'CREATE TABLE IF NOT EXISTS "calendly"."config" (id int)',
+	sql_down: '',
+	enabled: true
+}
+const deps = { reviewMigrations: async () => [MIGRATION], hasEeLicense: false }
+
+describe('a failed migration', () => {
+	beforeEach(() => {
+		outcome.migrationOk = true
+	})
+
+	it('marks the migrate row failed and leaves the run retryable', async () => {
+		outcome.migrationOk = false
+		const run = new ImportExecution(PLAN, deps as any)
+		await run.run()
+		expect(run.tasks.find((t) => t.key === 'migrate')?.status).toBe('failed')
+		// The regression: the row said failed while `error` stayed unset, so the page took
+		// the finished-run path and offered no way to run it again.
+		expect(run.error).toBeTruthy()
+		expect(run.error).toContain('migration')
+	})
+
+	it('says nothing went wrong when the migration succeeds', async () => {
+		const run = new ImportExecution(PLAN, deps as any)
+		await run.run()
+		expect(run.tasks.find((t) => t.key === 'migrate')?.status).toBe('done')
+		expect(run.error).toBeFalsy()
+	})
+})