From 2b50aeddc32b67eda6e2a329bac7dbb3ab5ac42c Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Tue, 25 Aug 2026 17:03:44 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20address=20round=209=20=E2=80=94=20abando?= =?UTF-8?q?n=20during=20the=20probe,=20and=20copy=20that=20outlived=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Abandoning while probeImportedPaths was in flight returned without settling anything. `import` goes running before the probe is asked, so the checklist kept a spinner on a run that had stopped, beside an enabled Retry and with no explanation. The settling the post-installProject path already did is now a helper both paths call. Three pieces of copy still described the behaviour this branch replaced: the resource alert said an existing path is "reported as failed" when the probe now leaves it alone and reports it as already there; and the step-4 footer and skip confirmation both told the user to set up a data table that the new `unknown` state means they already set up — only its schema could not be read. Those two now branch, so the strong warning stays strong for a data table that genuinely does not exist. The presence-key doc named `trigger:http_trigger`; WorkspaceTriggerKind has no such value. It is `http`, in the comment and in the two test mocks. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Xg8vXUuHCH3aRkf91sfxjx --- .../lib/components/ImportProjectStep.svelte | 5 +- .../src/lib/components/ImportSetupStep.svelte | 56 ++++++++++++++----- .../workspaceSettings/projectInstall.ts | 9 +-- .../projectInstallPresence.test.ts | 4 +- frontend/src/lib/importWizard/abandon.test.ts | 35 +++++++++++- .../src/lib/importWizard/execution.svelte.ts | 37 ++++++++---- .../src/lib/importWizard/probePaging.test.ts | 4 +- 7 files changed, 114 insertions(+), 36 deletions(-) diff --git a/frontend/src/lib/components/ImportProjectStep.svelte b/frontend/src/lib/components/ImportProjectStep.svelte index 1af3b61f5c..04c169d41c 100644 --- a/frontend/src/lib/components/ImportProjectStep.svelte +++ b/frontend/src/lib/components/ImportProjectStep.svelte @@ -374,8 +374,9 @@ bgClass="border-0" collapsible > - Resources are imported as empty stubs — set their values after import; a resource whose path - already exists is reported as failed (existing values are never overwritten). Trigger kinds are + Resources are imported as empty stubs — set their values after import; one whose path is + already in the workspace is left exactly as it is and reported as already there, so a value + you have since filled in is never overwritten. Trigger kinds are recreated disabled, except GCP and Azure triggers, which manage cloud subscriptions at creation and must be re-created manually after filling their resource. Kafka, NATS, SQS, GCP and Azure triggers all require Enterprise. Triggers that reference a resource depend on stubs imported diff --git a/frontend/src/lib/components/ImportSetupStep.svelte b/frontend/src/lib/components/ImportSetupStep.svelte index 6ebb9e94ad..f1b4662764 100644 --- a/frontend/src/lib/components/ImportSetupStep.svelte +++ b/frontend/src/lib/components/ImportSetupStep.svelte @@ -81,6 +81,11 @@ let resourceEditor: ResourceEditorDrawer | undefined = $state(undefined) const pendingTables = $derived(rows.filter((r) => r.status !== 'done')) + // Split because the two say different things to the user: one data table was never + // created, the other exists and could not be read. Telling someone to set up what they + // have already set up is how a warning stops being believed. + const unmadeTables = $derived(rows.filter((r) => r.status === 'unconfigured')) + const uncheckedTables = $derived(rows.filter((r) => r.status === 'unknown')) /** Rows the user has not dealt with, of either kind. */ const outstanding = $derived(pendingTables.length + blanks.filter((b) => !b.done).length) @@ -357,16 +362,24 @@ // element would otherwise run script in this authenticated origin. const names = pendingTables.map((r) => escapeHtml(r.name)).join(', ') const one = pendingTables.length === 1 + // An `unknown` row is set up — only its schema could not be read — so it cannot be + // told it is "not set up". Where the list mixes the two, say the weaker thing that + // is true of both rather than the stronger one that is false of half. + const anyUnmade = unmadeTables.length > 0 const confirmed = await confirmationModal.ask({ - title: 'The project will not run', + title: anyUnmade ? 'The project will not run' : 'This has not been verified', confirmationText: 'Skip anyway', - type: 'danger', - children: - `${one ? 'The data table' : 'The data tables'} ${names} ` + - `${one ? 'is' : 'are'} not set up, so the tables this project's apps and flows read ` + - `do not exist. Every one of them will fail as soon as it opens.

` + - `Setting ${one ? 'it' : 'them'} up later from workspace settings creates the ` + - `connection but not the tables — only this step runs the project's migration.` + type: anyUnmade ? 'danger' : 'info', + children: anyUnmade + ? `${one ? 'The data table' : 'The data tables'} ${names} ` + + `${one ? 'is' : 'are'} not set up, so the tables this project's apps and flows read ` + + `do not exist. Every one of them will fail as soon as it opens.

` + + `Setting ${one ? 'it' : 'them'} up later from workspace settings creates the ` + + `connection but not the tables — only this step runs the project's migration.` + : `${one ? 'The data table' : 'The data tables'} ${names} ` + + `${one ? 'is' : 'are'} set up, but ${one ? 'its' : 'their'} schema could not be ` + + `read, so whether this project's tables exist is unknown. Its apps and flows will ` + + `fail wherever they query a table that is missing.` }) if (!confirmed) return } @@ -590,12 +603,27 @@ Everything this project needs is configured. Finish, and it is ready to run. {:else if pendingTables.length > 0} - - {pendingTables.length === 1 ? 'This data table does not' : 'These data tables do not'} exist - yet, and the project's apps and flows query - {pendingTables.length === 1 ? 'tables inside it' : 'tables inside them'}. Until - {pendingTables.length === 1 ? 'it is' : 'they are'} set up, every one of them fails as soon as - it opens. + 0 + ? 'The project will not run without this' + : 'This could not be checked'} + size="xs" + > + {#if unmadeTables.length > 0} + {unmadeTables.length === 1 ? 'This data table does not' : 'These data tables do not'} exist + yet, and the project's apps and flows query + {unmadeTables.length === 1 ? 'tables inside it' : 'tables inside them'}. Until + {unmadeTables.length === 1 ? 'it is' : 'they are'} set up, every one of them fails as soon + as it opens. + {/if} + {#if uncheckedTables.length > 0} + {#if unmadeTables.length > 0}

{/if} + {uncheckedTables.length === 1 ? 'One data table is' : 'Some data tables are'} set up, but + {uncheckedTables.length === 1 ? 'its' : 'their'} schema could not be read, so whether the + project's tables are there is unknown. Running the migrations again is safe — they create + nothing that already exists. + {/if} {:else} diff --git a/frontend/src/lib/components/workspaceSettings/projectInstall.ts b/frontend/src/lib/components/workspaceSettings/projectInstall.ts index 6850797a80..6fd4bdd728 100644 --- a/frontend/src/lib/components/workspaceSettings/projectInstall.ts +++ b/frontend/src/lib/components/workspaceSettings/projectInstall.ts @@ -266,10 +266,11 @@ export async function applyOneMigration( /** * The kinds an import writes that carry a path and can therefore already be there. * - * Triggers carry their own kind too (`trigger:schedule`, `trigger:http_trigger`, …): each - * trigger kind is a separate table keyed on `(path, workspace_id)`, so one workspace can hold - * a schedule and an HTTP trigger both called `f/cal/sync`. Flattening them to `trigger` would - * let whichever exists answer for the other. + * Triggers carry their own kind too (`trigger:schedule`, `trigger:http`, … — the values of + * `WorkspaceTriggerKind`): each trigger kind is a separate table keyed on + * `(path, workspace_id)`, so one workspace can hold a schedule and an HTTP trigger both + * called `f/cal/sync`. Flattening them to `trigger` would let whichever exists answer for + * the other. */ export type ImportedKind = | 'script' diff --git a/frontend/src/lib/components/workspaceSettings/projectInstallPresence.test.ts b/frontend/src/lib/components/workspaceSettings/projectInstallPresence.test.ts index 2e69dca81a..186d581d58 100644 --- a/frontend/src/lib/components/workspaceSettings/projectInstallPresence.test.ts +++ b/frontend/src/lib/components/workspaceSettings/projectInstallPresence.test.ts @@ -24,7 +24,7 @@ vi.mock('$lib/gen', () => { vi.mock('../triggers/workspaceTriggersList', () => ({ TRIGGER_KINDS: { schedule: { badge: 'Schedule', resourceField: undefined }, - http_trigger: { badge: 'HTTP', resourceField: undefined } + http: { badge: 'HTTP', resourceField: undefined } }, createWorkspaceTriggerDisabled: vi.fn(async (_ws: string, t: { path: string; kind: string }) => { created.triggers.push(`${t.kind}:${t.path}`) @@ -99,7 +99,7 @@ describe('installProject presence', () => { // hold a schedule and an HTTP trigger both called `f/calendly/nightly`. The one that // exists must not answer for the one that does not. it('does not let another trigger kind at the same path mask this one', async () => { - const results = await run(new Set([presenceKey('trigger:http_trigger', 'f/calendly/nightly')])) + const results = await run(new Set([presenceKey('trigger:http', 'f/calendly/nightly')])) expect(created.triggers).toEqual(['schedule:f/calendly/nightly']) expect(results).toContainEqual({ path: 'f/calendly/nightly', ok: true }) }) diff --git a/frontend/src/lib/importWizard/abandon.test.ts b/frontend/src/lib/importWizard/abandon.test.ts index 09e31b8aca..abf0ff61b9 100644 --- a/frontend/src/lib/importWizard/abandon.test.ts +++ b/frontend/src/lib/importWizard/abandon.test.ts @@ -18,11 +18,18 @@ vi.mock('$lib/gen', () => ({ vi.mock('$lib/storeUtils', () => ({ switchWorkspace: vi.fn() })) // What the destination already holds. A test sets this to stand in for a workspace that has // some of the bundle in it — a half-finished run, or an existing workspace. -const present = vi.hoisted(() => ({ paths: new Set() })) +const present = vi.hoisted(() => ({ + paths: new Set(), + /** Fires when the probe is entered, so a test can abandon while it is in flight. */ + onProbe: undefined as (() => void) | undefined +})) vi.mock('./probe', async (orig) => ({ ...(await orig()), probeWorkspace: vi.fn(async () => ({ exists: false, ours: false })), - probeImportedPaths: vi.fn(async () => present.paths) + probeImportedPaths: vi.fn(async () => { + present.onProbe?.() + return present.paths + }) })) vi.mock('$lib/user', () => ({ getUserExt: vi.fn(async () => ({ username: 'u' })) })) // Let a test abandon *during* a write loop, which is the only way it happens for real: @@ -204,3 +211,27 @@ describe('retrying over what is already there', () => { expect(run.tasks.find((t) => t.key === 'import')?.detail).toMatch(/3 imported/) }) }) + +describe('abandoning while the presence probe is in flight', () => { + beforeEach(() => { + present.paths = new Set() + present.onProbe = undefined + }) + + /** + * `import` goes `running` before the probe is asked, so returning straight out of an + * abandon here leaves a spinner on a run that has stopped — next to an enabled Retry and + * with no explanation of why it stopped. + */ + it('leaves no task running', async () => { + const run = new ImportExecution(PLAN, deps as any) + // Abandoned from inside the probe: `import` is already `running` by then, and the + // executor's next look at the flag is the early return under test. + present.onProbe = () => run.abandon() + await run.run() + expect(run.tasks.find((t) => t.status === 'running')).toBeUndefined() + expect(run.tasks.find((t) => t.key === 'import')?.status).toBe('failed') + expect(run.error).toBeTruthy() + expect(run.done).toBe(false) + }) +}) diff --git a/frontend/src/lib/importWizard/execution.svelte.ts b/frontend/src/lib/importWizard/execution.svelte.ts index 998cb13757..06c33f3222 100644 --- a/frontend/src/lib/importWizard/execution.svelte.ts +++ b/frontend/src/lib/importWizard/execution.svelte.ts @@ -324,6 +324,26 @@ export class ImportExecution { } } + /** + * Leave the checklist saying what actually happened, from wherever the run stopped. + * + * Reached from every point after `import` goes `running`, so nothing is left spinning on a + * run that has ended. A partial import is failed rather than done: calling it done reports + * a clean import over items that never started, and the resumed step offers Continue where + * it should offer Retry. + */ + #settleAbandoned() { + 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.' + } + async #import(workspace: string, exportData: ProjectExport): Promise { this.#set('import', 'running') const folder = this.#plan.folder?.trim() || exportData.project.slug @@ -366,7 +386,12 @@ export class ImportExecution { triggers: exportData.triggers.length > 0, hasEeLicense: this.#deps.hasEeLicense }) - if (this.#abandoned) return + // Settled, not just returned: `import` has been `running` since before the probe, and + // leaving it there shows a spinner on a run that has stopped, next to a Retry button. + if (this.#abandoned) { + this.#settleAbandoned() + return + } try { await installProject({ alreadyPresent, @@ -392,15 +417,7 @@ export class ImportExecution { // what it got through; calling that `done` reports a clean import over items that // never started, and the resumed step would offer Continue instead of Retry. 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.' + this.#settleAbandoned() return } diff --git a/frontend/src/lib/importWizard/probePaging.test.ts b/frontend/src/lib/importWizard/probePaging.test.ts index 06e64e93ec..17e7fb9a11 100644 --- a/frontend/src/lib/importWizard/probePaging.test.ts +++ b/frontend/src/lib/importWizard/probePaging.test.ts @@ -66,9 +66,9 @@ describe('trigger presence keys', () => { * key on the path alone and the one that exists reports the other as already imported. */ it('keys a trigger by its kind, so one kind cannot answer for another', async () => { - triggerRows.rows = [{ kind: 'http_trigger', path: 'f/calendly/sync' }] + triggerRows.rows = [{ kind: 'http', path: 'f/calendly/sync' }] const found = await probeImportedPaths('w', 'calendly', { triggers: true }) - expect(found.has(presenceKey('trigger:http_trigger', 'f/calendly/sync'))).toBe(true) + expect(found.has(presenceKey('trigger:http', 'f/calendly/sync'))).toBe(true) // The schedule the project also ships at that path has not been imported. expect(found.has(presenceKey('trigger:schedule', 'f/calendly/sync'))).toBe(false) })