From f9ad196712dccbf774a1bc9bd17ab13dc98b901f Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Mon, 24 Aug 2026 11:52:59 +0200 Subject: [PATCH] feat: import only what the destination does not already have MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A retry resent the whole bundle. Everything that had already landed came back as "already exists" — nine failures over work that had succeeded, with no way to tell those from the ones that genuinely failed. The same thing happened importing into a workspace that already held some of the project. `installProject` now takes `alreadyPresent`, checked after retargeting because that is what the items will actually be called, and `probeImportedPaths` fills it from the destination on every run. On a workspace the run just created the answer is empty and nothing is skipped, so this costs four scoped reads and changes nothing about a first import. Skipping is not replacing. An item that is there is left exactly as it is — the same promise `updateIfExists: false` already makes for a resource whose value someone has since filled in. `InstallResult` gains `skipped`, because "already there" is neither an import nor a failure and reporting it as either is a lie. The checklist still lists every item the project ships; a skipped one shows as skipped and says why. The import row now counts the three outcomes separately — `8 already there` rather than a green tick over `2 apps, 4 scripts, 2 resources` it did not write. That last part needed the pre-run breakdown to stand down once the run has an outcome of its own, or it went on claiming the import had happened. Checked by removing the gate: two of the four new tests fail. Verified against a real backend by re-importing Calendly into a workspace that already had it — 0 failures, 0 create requests, and the row reads "8 already there", where the same run previously produced 9 conflicts. Co-Authored-By: Claude Opus 5 (1M context) --- .../lib/components/ImportProjectStep.svelte | 16 ++++- .../workspaceSettings/projectInstall.ts | 33 ++++++++++- frontend/src/lib/importWizard/abandon.test.ts | 58 +++++++++++++++++++ .../src/lib/importWizard/execution.svelte.ts | 24 ++++++-- 4 files changed, 121 insertions(+), 10 deletions(-) diff --git a/frontend/src/lib/components/ImportProjectStep.svelte b/frontend/src/lib/components/ImportProjectStep.svelte index c8b02621e9..fb9beccb36 100644 --- a/frontend/src/lib/components/ImportProjectStep.svelte +++ b/frontend/src/lib/components/ImportProjectStep.svelte @@ -179,7 +179,11 @@ // total said in a more useful way. const checklist = $derived( tasks.map((task) => { - const detail = (task.key === 'import' && importSummary) || task.detail + // The breakdown says what the import *will* bring, so it belongs to the row only + // until the run has an outcome of its own. Left in place it would go on claiming + // "2 apps, 4 scripts" over a run that wrote none of them because they were + // already there. + const detail = task.key === 'import' ? task.detail || importSummary : task.detail return { title: detail ? `${task.label} — ${detail}` : task.label, status: task.status, @@ -189,8 +193,14 @@ task.key === 'import' ? execution?.itemResults.map((r) => ({ title: r.path, - status: r.ok ? ('done' as const) : ('failed' as const), - description: r.error + // `skipped`, not `done`: nothing was written, and a green tick over an + // item this run left alone claims an import that did not happen. + status: !r.ok + ? ('failed' as const) + : r.skipped + ? ('skipped' as const) + : ('done' as const), + description: r.skipped ? 'Already in the workspace — left as it is.' : r.error })) : undefined } diff --git a/frontend/src/lib/components/workspaceSettings/projectInstall.ts b/frontend/src/lib/components/workspaceSettings/projectInstall.ts index 994985947d..b582c492b0 100644 --- a/frontend/src/lib/components/workspaceSettings/projectInstall.ts +++ b/frontend/src/lib/components/workspaceSettings/projectInstall.ts @@ -44,6 +44,11 @@ export interface InstallResult { path: string ok: boolean error?: string + /** + * Already in the destination, so nothing was written. Not a failure and not an import — + * reporting it as either would be a lie, and the difference is what a retry is for. + */ + skipped?: boolean } // Guarding an item's own path is not enough: the `$res:`/script/flow refs baked @@ -273,6 +278,16 @@ export async function installProject(args: { * service call: the import wizard uses it when the user confirms leaving mid-run. */ stopped?: () => boolean + /** + * Paths already in the destination, retargeted — so a retry writes only what is missing + * instead of replaying the bundle into a wall of "already exists". Compared after + * retargeting, because that is what these items will actually be called. + * + * Never a way to *replace* anything: an item that is there is left exactly as it is, + * which is the same promise `updateIfExists: false` makes for a resource whose value + * someone has since filled in. + */ + alreadyPresent?: Set hasEeLicense: boolean onResult: (r: InstallResult) => void }): Promise { @@ -284,7 +299,8 @@ export async function installProject(args: { hasEeLicense, onResult, onMigrationsStart, - stopped + stopped, + alreadyPresent } = args const record = (path: string, p: Promise): Promise => @@ -296,6 +312,17 @@ export async function installProject(args: { /** Every write goes through here, so one check covers items, variables and migrations. */ const halted = () => stopped?.() === true + /** + * True when the destination already has this path, so the write is not attempted. Reported + * rather than dropped: the checklist has to account for every item the project ships, and + * "already there" is a different thing from "imported". + */ + const present = (path: string): boolean => { + if (!alreadyPresent?.has(path)) return false + onResult({ path, ok: true, skipped: true }) + return true + } + try { await FolderService.createFolder({ workspace, requestBody: { name: folder } }) } catch {} @@ -336,6 +363,7 @@ export async function installProject(args: { for (const s of proj.scripts) { if (halted()) return + if (present(s.path)) continue // `$var:` is resolved in job args (flow inputs, schedule args, trigger config), // not in script source, so there is no variable arg to contain here. await checkedItem(s.path, extractScriptRefs(s.content ?? ''), undefined, () => @@ -344,10 +372,12 @@ export async function installProject(args: { } for (const f of proj.flows) { if (halted()) return + if (present(f.path)) continue await checkedItem(f.path, extractFlowRefs(f.value), f.value, () => importFlow(workspace, f)) } for (const r of proj.resources) { if (halted()) return + if (present(r.path)) continue await checked(r.path, () => importResourceStub(workspace, r)) } // Placeholders for the project's internal `$var:`/`$jsonvar:` refs (retargeted @@ -360,6 +390,7 @@ export async function installProject(args: { } for (const a of proj.apps) { if (halted()) return + if (present(a.path)) continue const isRaw = a.app_type === 'raw' const refs = isRaw ? extractRawAppRefs(a.value?.raw ?? '') : extractAppRefs(a.value) // Raw apps hold their runnables in the `value.raw` JSON string; parse it so the diff --git a/frontend/src/lib/importWizard/abandon.test.ts b/frontend/src/lib/importWizard/abandon.test.ts index ab9e3b62d4..66942a1edf 100644 --- a/frontend/src/lib/importWizard/abandon.test.ts +++ b/frontend/src/lib/importWizard/abandon.test.ts @@ -16,6 +16,14 @@ 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() })) +vi.mock('./probe', async (orig) => ({ + ...(await orig()), + probeWorkspace: vi.fn(async () => ({ exists: false, ours: false })), + probeImportedPaths: vi.fn(async () => 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: // `run()` clears the flag on entry so a retry can proceed. Two hooks, because the item and @@ -32,6 +40,10 @@ vi.mock('$lib/components/workspaceSettings/projectInstall', () => ({ // returns on success. for (const path of ['a', 'b', 'c']) { if (args.stopped?.() === true) return + if (args.alreadyPresent?.has(path)) { + args.onResult({ path, ok: true, skipped: true }) + continue + } args.onResult({ path, ok: true }) hooks.afterFirstItem?.() hooks.afterFirstItem = undefined @@ -109,6 +121,7 @@ describe('abandoning mid-import', () => { beforeEach(() => { hooks.afterFirstItem = undefined hooks.afterMigrationsStart = undefined + present.paths = new Set() }) it('does not report done, so the resumed step offers Retry rather than Continue', async () => { @@ -143,3 +156,48 @@ describe('abandoning mid-import', () => { expect(run.done).toBe(false) }) }) + +/** + * A retry used to resend the whole bundle, so everything that had already landed came back as + * "already exists" — a wall of failures over work that had succeeded. What is already there is + * now skipped, and reported as skipped rather than as imported. + */ +describe('retrying over what is already there', () => { + beforeEach(() => { + present.paths = new Set() + }) + + it('writes nothing for a path the destination already holds', async () => { + present.paths = new Set(['a', 'b']) + const run = new ImportExecution(PLAN, deps) + await run.run() + const byPath = new Map(run.itemResults.map((r) => [r.path, r])) + expect(byPath.get('a')?.skipped).toBe(true) + expect(byPath.get('b')?.skipped).toBe(true) + expect(byPath.get('c')?.skipped).toBeUndefined() + }) + + it('still accounts for every item, so the checklist stays complete', async () => { + present.paths = new Set(['a', 'b']) + const run = new ImportExecution(PLAN, deps) + await run.run() + expect(run.itemResults.length).toBe(3) + expect(run.done).toBe(true) + }) + + it('says what it did rather than claiming to have imported all of it', async () => { + present.paths = new Set(['a', 'b']) + const run = new ImportExecution(PLAN, deps) + await run.run() + const importRow = run.tasks.find((t) => t.key === 'import') + expect(importRow?.detail).toMatch(/1 imported/) + expect(importRow?.detail).toMatch(/2 already there/) + }) + + it('imports everything when the destination is empty', async () => { + const run = new ImportExecution(PLAN, deps) + await run.run() + expect(run.itemResults.every((r) => !r.skipped)).toBe(true) + expect(run.tasks.find((t) => t.key === 'import')?.detail).toMatch(/3 imported/) + }) +}) diff --git a/frontend/src/lib/importWizard/execution.svelte.ts b/frontend/src/lib/importWizard/execution.svelte.ts index 503695a18b..ff7e3171ad 100644 --- a/frontend/src/lib/importWizard/execution.svelte.ts +++ b/frontend/src/lib/importWizard/execution.svelte.ts @@ -13,7 +13,7 @@ import type { ProjectMigration } from '$lib/components/workspaceSettings/projectBundle' import { planWorkspaceId, type ImportPlan } from './plan' -import { probeWorkspace } from './probe' +import { probeImportedPaths, probeWorkspace } from './probe' /** * The only thing in the wizard that changes anything. It takes a finished plan and @@ -359,8 +359,15 @@ export class ImportExecution { } this.results = [] + // Asked every run, not only on a retry: the destination may be a workspace that already + // holds some of these paths, and a run interrupted halfway is indistinguishable from + // one that never started. On a workspace this run just created the answer is empty and + // nothing is skipped. + const alreadyPresent = await probeImportedPaths(workspace, folder) + if (this.#abandoned) return try { await installProject({ + alreadyPresent, workspace, exportData, folder, @@ -397,11 +404,16 @@ export class ImportExecution { const items = this.itemResults const failed = items.filter((r) => !r.ok).length - this.#set( - 'import', - failed > 0 ? 'failed' : 'done', - failed > 0 ? `${items.length - failed} of ${items.length} imported` : `${items.length} items` - ) + const skipped = items.filter((r) => r.skipped).length + // Three outcomes, so the row says which: written, left alone because it was already + // there, and failed. Rolling the second into the first would report an import that + // did not happen. + const wrote = items.length - failed - skipped + const parts: string[] = [] + if (wrote > 0 || (failed === 0 && skipped === 0)) parts.push(`${wrote} imported`) + if (skipped > 0) parts.push(`${skipped} already there`) + if (failed > 0) parts.push(`${failed} failed`) + this.#set('import', failed > 0 ? 'failed' : 'done', parts.join(', ')) const migrated = this.migrationResults if (migrated.length) {