mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-11 08:07:15 +00:00
fix: address review round 4 — unreviewed SQL, premature finish, stale Back
**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>
This commit is contained in:
co-authored by
Claude Opus 5
parent
ff8326c3c0
commit
a07a63ae5c
@@ -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
|
||||
|
||||
@@ -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>
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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>
|
||||
|
||||
Reference in New Issue
Block a user