fix(drafts): stop mis-filing workspace-blind legacy drafts on migration (#9725)

frontend/src/lib/userDraftDbMigration.ts

@@ -1,11 +1,12 @@
 /**
  * One-off migration from the localStorage UserDraft autosave to the
- * DB-backed `draft` table. Runs after `migrateLegacyUserDrafts` (which
- * produces the `userdraft/w/{workspace}/{kind}/{path}` keys this reads),
- * POSTing each to `/drafts/update` and clearing the source key only on
- * success — so it's idempotent without a sentinel; failed entries retry next
- * mount. Not workspace-gated: keys embed their own workspace and the token
- * covers all of them, so gating would orphan other-workspace entries.
+ * DB-backed `draft` table. Reads the workspace-scoped
+ * `userdraft/w/{workspace}/{kind}/{path}` keys (written by the editor during
+ * the interim LS-backed phase, so the embedded workspace is correct), POSTing
+ * each to `/drafts/update` and clearing the source key only on success — so
+ * it's idempotent without a sentinel; failed entries retry next mount. Not
+ * workspace-gated: keys embed their own workspace and the token covers all of
+ * them, so gating would orphan other-workspace entries.
  *
  * Before uploading, each draft is compared against its deployed version
  * (script / flow / app); a draft that's deep-equal to what's deployed carries

frontend/src/lib/userDraftLegacyMigration.test.ts

@@ -1,6 +1,6 @@
 import { describe, it, expect, beforeEach } from 'vitest'
 import {
-	migrateLegacyUserDrafts,
+	purgeLegacyUserDrafts,
 	__resetUserDraftLegacyMigrationForTesting
 } from './userDraftLegacyMigration'
 
@@ -8,18 +8,12 @@ function encodeLegacy(value: unknown): string {
 	return btoa(encodeURIComponent(JSON.stringify(value)))
 }
 
-function wrapped<V>(value: V): string {
-	return JSON.stringify({ value })
-}
-
-// Read a migrated entry, strip the GC `lastWrittenAt` stamp so assertions
-// can match the `{ value }` shape regardless of when the migration ran.
-function storedShape(key: string): string | null {
-	const raw = localStorage.getItem(key)
-	if (raw == null) return null
-	const parsed = JSON.parse(raw)
-	delete parsed.lastWrittenAt
-	return JSON.stringify(parsed)
+const legacyApp = {
+	grid: [],
+	fullscreen: false,
+	theme: undefined,
+	unusedInlineScripts: [],
+	hiddenInlineScripts: []
 }
 
 beforeEach(() => {
@@ -27,197 +21,110 @@ beforeEach(() => {
 	__resetUserDraftLegacyMigrationForTesting()
 })
 
-describe('migrateLegacyUserDrafts', () => {
-	it('migrates a legacy app draft to the workspace-scoped key with a { value } wrapper', () => {
-		// Shape mirrors what the legacy AppEditor wrote: `encodeState($appStore)`,
-		// i.e. the inner App value, not the wrapping AppWithLastVersion.
-		const legacyApp = {
-			grid: [],
-			fullscreen: false,
-			theme: undefined,
-			unusedInlineScripts: [],
-			hiddenInlineScripts: []
-		}
+describe('purgeLegacyUserDrafts', () => {
+	it('drops a recognised legacy app draft without re-creating it under any key', () => {
 		localStorage.setItem('app-u/me/dashboard', encodeLegacy(legacyApp))
 
-		migrateLegacyUserDrafts('main')
+		purgeLegacyUserDrafts()
 
 		expect(localStorage.getItem('app-u/me/dashboard')).toBeNull()
-		expect(storedShape('userdraft/w/main/app/u/me/dashboard')).toBe(wrapped(legacyApp))
+		// The workspace-blind key is gone, NOT promoted to a guessed workspace.
+		expect(localStorage.getItem('userdraft/w/main/app/u/me/dashboard')).toBeNull()
 	})
 
-	it('migrates a legacy empty-path app draft (the `app` literal key)', () => {
-		const legacyApp = {
-			grid: [],
-			fullscreen: false,
-			unusedInlineScripts: [],
-			hiddenInlineScripts: []
-		}
+	it('drops the empty-path legacy keys (`app` / `flow` / `rawapp` literals)', () => {
 		localStorage.setItem('app', encodeLegacy(legacyApp))
+		localStorage.setItem('flow', encodeLegacy({ flow: { summary: '', value: { modules: [] } } }))
+		localStorage.setItem('rawapp', encodeLegacy({ files: {}, runnables: {}, data: {} }))
 
-		migrateLegacyUserDrafts('main')
+		purgeLegacyUserDrafts()
 
 		expect(localStorage.getItem('app')).toBeNull()
-		expect(storedShape('userdraft/w/main/app/')).toBe(wrapped(legacyApp))
+		expect(localStorage.getItem('flow')).toBeNull()
+		expect(localStorage.getItem('rawapp')).toBeNull()
 	})
 
-	it('migrates a legacy flow draft and strips the view-state envelope', () => {
-		const flow = { summary: 'f', value: { modules: [] }, path: 'u/me/myflow' }
-		const legacyBundle = {
-			flow,
-			path: 'u/me/myflow',
-			selectedId: 'settings',
-			draft_triggers: [{ id: 't1' }],
-			selected_trigger: null,
-			loadedFromHistory: undefined
-		}
-		localStorage.setItem('flow-u/me/myflow', encodeLegacy(legacyBundle))
+	it('drops recognised legacy flow and raw-app drafts', () => {
+		localStorage.setItem(
+			'flow-u/me/myflow',
+			encodeLegacy({ flow: { summary: 'f', value: { modules: [] } }, selectedId: 'settings' })
+		)
+		localStorage.setItem(
+			'rawapp-u/me/site',
+			encodeLegacy({ files: { 'index.tsx': 'x' }, runnables: {}, data: {} })
+		)
 
-		migrateLegacyUserDrafts('main')
+		purgeLegacyUserDrafts()
 
 		expect(localStorage.getItem('flow-u/me/myflow')).toBeNull()
-		// Only the inner Flow survives; the view-state envelope is dropped.
-		expect(storedShape('userdraft/w/main/flow/u/me/myflow')).toBe(wrapped(flow))
-	})
-
-	it('migrates a legacy raw-app draft, defaulting the new `summary` field', () => {
-		const legacy = {
-			files: { 'index.tsx': 'export default () => null' },
-			runnables: {},
-			data: { tables: [] }
-		}
-		localStorage.setItem('rawapp-u/me/site', encodeLegacy(legacy))
-
-		migrateLegacyUserDrafts('main')
-
 		expect(localStorage.getItem('rawapp-u/me/site')).toBeNull()
-		expect(storedShape('userdraft/w/main/raw_app/u/me/site')).toBe(
-			wrapped({ ...legacy, summary: '' })
-		)
 	})
 
-	it('preserves an existing new-format entry instead of overwriting it', () => {
-		// Old and new both exist for the same item — the new one is presumed
-		// fresher.
-		localStorage.setItem(
-			'app-u/me/dash',
-			encodeLegacy({
-				grid: [],
-				fullscreen: false,
-				unusedInlineScripts: [],
-				hiddenInlineScripts: []
-			})
-		)
-		const existingNew = wrapped({ value: 'new' })
-		localStorage.setItem('userdraft/w/main/app/u/me/dash', existingNew)
+	it('leaves the workspace-scoped interim keys untouched (migrateUserDraftsToDb owns those)', () => {
+		const interim = JSON.stringify({ value: { modules: [] } })
+		localStorage.setItem('userdraft/w/main/flow/u/me/keep', interim)
 
-		migrateLegacyUserDrafts('main')
+		purgeLegacyUserDrafts()
 
-		expect(localStorage.getItem('app-u/me/dash')).toBeNull()
-		expect(localStorage.getItem('userdraft/w/main/app/u/me/dash')).toBe(existingNew)
-	})
-
-	it('is idempotent — the second invocation is a no-op', () => {
-		localStorage.setItem(
-			'app-u/me/dash',
-			encodeLegacy({
-				grid: [],
-				fullscreen: false,
-				unusedInlineScripts: [],
-				hiddenInlineScripts: []
-			})
-		)
-		migrateLegacyUserDrafts('main')
-		expect(localStorage.getItem('userdraft/w/main/app/u/me/dash')).not.toBeNull()
-
-		// Drop the migrated entry to detect any re-migration attempt.
-		localStorage.removeItem('userdraft/w/main/app/u/me/dash')
-		// Drop the source too, so re-running couldn't even find a source.
-		// (The sentinel alone should be enough; this just clarifies the intent.)
-		migrateLegacyUserDrafts('main')
-		expect(localStorage.getItem('userdraft/w/main/app/u/me/dash')).toBeNull()
-	})
-
-	it('skips entirely when no workspace is available', () => {
-		localStorage.setItem(
-			'app-u/me/dash',
-			encodeLegacy({
-				grid: [],
-				fullscreen: false,
-				unusedInlineScripts: [],
-				hiddenInlineScripts: []
-			})
-		)
-		migrateLegacyUserDrafts('')
-
-		expect(localStorage.getItem('app-u/me/dash')).not.toBeNull()
-	})
-
-	it('handles malformed legacy payloads without throwing', () => {
-		localStorage.setItem('app-u/me/garbled', 'not-base64!!!')
-		expect(() => migrateLegacyUserDrafts('main')).not.toThrow()
-		// Migration didn't migrate, didn't crash — leaves the entry alone.
-		expect(localStorage.getItem('app-u/me/garbled')).toBe('not-base64!!!')
+		expect(localStorage.getItem('userdraft/w/main/flow/u/me/keep')).toBe(interim)
 	})
 
 	it('leaves keys whose path does not match the legacy `u|f/owner/name` shape alone', () => {
-		// A future feature or neighbouring code might pick a key like
-		// `app-recent` for its own purposes. The path doesn't look like a
-		// Windmill item path, so the migration must skip it.
+		// `app-recent` / `app-some_other_app` look like the legacy prefix but the
+		// suffix isn't a Windmill item path — a future feature might own them.
 		localStorage.setItem('app-recent', 'whatever')
 		localStorage.setItem('app-some_other_app', 'whatever')
-		// `flow-u/me/foo` matches the shape and would be migrated, but the
-		// payload also needs to look like a Windmill draft (asserted below).
-		localStorage.setItem('flow-u/me/foo', encodeLegacy({ flow: { value: { modules: [] } } }))
 
-		migrateLegacyUserDrafts('main')
+		purgeLegacyUserDrafts()
 
 		expect(localStorage.getItem('app-recent')).toBe('whatever')
 		expect(localStorage.getItem('app-some_other_app')).toBe('whatever')
-		expect(localStorage.getItem('userdraft/w/main/flow/u/me/foo')).not.toBeNull()
 	})
 
-	it('skips legacy-shaped keys whose payload does not look like a Windmill draft', () => {
-		// `app-u/me/dash` matches LEGACY_PATH_SHAPE and decodes to valid JSON,
-		// but none of the App-shape fields (grid/fullscreen/theme/
-		// unusedInlineScripts/hiddenInlineScripts) are present. Treat it as
-		// unrelated and leave it untouched.
-		const unrelated = encodeLegacy({ random: 'data', count: 7 })
-		localStorage.setItem('app-u/me/dash', unrelated)
+	it('leaves legacy-shaped keys whose payload does not look like a Windmill draft', () => {
+		// Matches LEGACY_PATH_SHAPE and decodes to valid JSON, but carries none of
+		// the App/flow draft fields — treat as unrelated, do not delete.
+		const unrelatedApp = encodeLegacy({ random: 'data', count: 7 })
+		localStorage.setItem('app-u/me/dash', unrelatedApp)
 		const unrelatedFlow = encodeLegacy({ stepsState: {} })
 		localStorage.setItem('flow-u/me/bar', unrelatedFlow)
 
-		migrateLegacyUserDrafts('main')
+		purgeLegacyUserDrafts()
 
-		expect(localStorage.getItem('app-u/me/dash')).toBe(unrelated)
-		expect(localStorage.getItem('userdraft/w/main/app/u/me/dash')).toBeNull()
+		expect(localStorage.getItem('app-u/me/dash')).toBe(unrelatedApp)
 		expect(localStorage.getItem('flow-u/me/bar')).toBe(unrelatedFlow)
-		expect(localStorage.getItem('userdraft/w/main/flow/u/me/bar')).toBeNull()
 	})
 
-	it('migrates multiple legacy entries in a single invocation', () => {
-		localStorage.setItem(
-			'app-u/me/a',
-			encodeLegacy({
-				grid: [],
-				fullscreen: false,
-				unusedInlineScripts: [],
-				hiddenInlineScripts: []
-			})
-		)
+	it('leaves a malformed (non-base64) legacy payload alone and does not throw', () => {
+		localStorage.setItem('app-u/me/garbled', 'not-base64!!!')
+
+		expect(() => purgeLegacyUserDrafts()).not.toThrow()
+		expect(localStorage.getItem('app-u/me/garbled')).toBe('not-base64!!!')
+	})
+
+	it('is idempotent — once the sentinel is set, a later legacy key survives', () => {
+		localStorage.setItem('app-u/me/a', encodeLegacy(legacyApp))
+		purgeLegacyUserDrafts()
+		expect(localStorage.getItem('app-u/me/a')).toBeNull()
+
+		// A key written after the first run is NOT swept (the sentinel short-circuits).
+		localStorage.setItem('app-u/me/b', encodeLegacy(legacyApp))
+		purgeLegacyUserDrafts()
+		expect(localStorage.getItem('app-u/me/b')).not.toBeNull()
+	})
+
+	it('purges multiple legacy entries in a single invocation', () => {
+		localStorage.setItem('app-u/me/a', encodeLegacy(legacyApp))
 		localStorage.setItem(
 			'flow-u/me/b',
-			encodeLegacy({ flow: { summary: '', value: { modules: [] }, path: 'u/me/b' } })
-		)
-		localStorage.setItem(
-			'rawapp-u/me/c',
-			encodeLegacy({ files: {}, runnables: {}, data: { tables: [] } })
+			encodeLegacy({ flow: { summary: '', value: { modules: [] } } })
 		)
+		localStorage.setItem('rawapp-u/me/c', encodeLegacy({ files: {}, runnables: {}, data: {} }))
 
-		migrateLegacyUserDrafts('main')
+		purgeLegacyUserDrafts()
 
-		expect(localStorage.getItem('userdraft/w/main/app/u/me/a')).not.toBeNull()
-		expect(localStorage.getItem('userdraft/w/main/flow/u/me/b')).not.toBeNull()
-		expect(localStorage.getItem('userdraft/w/main/raw_app/u/me/c')).not.toBeNull()
+		expect(localStorage.getItem('app-u/me/a')).toBeNull()
+		expect(localStorage.getItem('flow-u/me/b')).toBeNull()
+		expect(localStorage.getItem('rawapp-u/me/c')).toBeNull()
 	})
 })

frontend/src/lib/userDraftLegacyMigration.ts

@@ -1,22 +1,29 @@
 /**
- * One-off migration from the pre-UserDraft localStorage autosave entries to
- * the workspace-scoped `userdraft/w/{ws}/{kind}/{path}` format.
+ * One-shot purge of the pre-UserDraft browser-local autosave keys.
  *
- * Legacy keys (global, not workspace-scoped — assumed to belong to the user's
- * current workspace at migration time):
+ * The original autosave (pre-#9121) wrote workspace-BLIND keys:
  *
- *   `flow`            / `flow-{path}`    base64 of `encodeState({ flow, path, selectedId, draft_triggers, ... })`
- *   `app`             / `app-{path}`     base64 of `encodeState(App)`
- *   `rawapp`          / `rawapp-{path}`  base64 of `encodeState({ files, runnables, data })`
+ *   `flow`   / `flow-{path}`    base64 of `encodeState({ flow, path, selectedId, draft_triggers, ... })`
+ *   `app`    / `app-{path}`     base64 of `encodeState(App)`
+ *   `rawapp` / `rawapp-{path}`  base64 of `encodeState({ files, runnables, data })`
  *
- * Target keys: `userdraft/w/{workspace}/{flow|app|raw_app}/{path}` storing
- * `JSON.stringify({ value: <transformed legacy value> })`.
+ * Neither the key nor the decoded value records a workspace (the value carries
+ * only workspace-agnostic item paths like `u/me/x`), so these drafts cannot be
+ * attributed to the workspace they were edited in. The current editors are
+ * DB-backed and never read these keys, so they are dead data with one dangerous
+ * property: promoting them to the DB would force a GUESS of the workspace, which
+ * mis-files drafts into whatever workspace happened to be active when the
+ * migration first ran (a single global sentinel gates it). We therefore drop
+ * them instead of migrating them.
+ *
+ * Only keys that BOTH match the legacy path shape AND decode to a plausible
+ * legacy draft are removed; unrelated look-alikes (`app-recent`, garbage,
+ * non-Windmill payloads) are left untouched. The workspace-scoped interim keys
+ * (`userdraft/w/{ws}/...`, written by the editor with the correct workspace)
+ * are NOT touched here — `migrateUserDraftsToDb` still pushes those to the DB.
  *
  * Idempotent: writes a sentinel under `MIGRATION_FLAG` after the first run so
- * subsequent invocations are no-ops. Existing new-format entries are never
- * overwritten — when both an old and a new entry exist for the same item, the
- * old one is simply dropped on the assumption that the new entry is the more
- * recent edit.
+ * subsequent invocations are no-ops.
  *
  * This file is intentionally standalone — it does not import from
  * `userDraft.svelte.ts` so the new code stays uncluttered by the legacy
@@ -71,10 +78,9 @@ function decodeLegacyState(raw: string): unknown {
  * Per-kind shape gate. The legacy keys (`app-foo`, `flow-foo`, ...) are
  * unusual enough that nothing else in the codebase has used them, but
  * matching `LEGACY_PATH_SHAPE` doesn't prove the payload is actually a
- * Windmill draft (any base64-of-JSON could pass). Promoting a stray payload
- * would silently surface as a phantom "Restored from local storage" toast
- * on the next edit, so we reject anything that doesn't carry the fields the
- * legacy writers actually produced.
+ * Windmill draft (any base64-of-JSON could pass). We only delete keys we can
+ * positively recognise as legacy drafts, so a stray look-alike that happens to
+ * use this key shape is left untouched rather than silently dropped.
  */
 function isPlausibleLegacyValue(kind: LegacyKind, decoded: unknown): boolean {
 	if (decoded == null || typeof decoded !== 'object') return false
@@ -103,32 +109,6 @@ function isPlausibleLegacyValue(kind: LegacyKind, decoded: unknown): boolean {
 	}
 }
 
-function transformLegacyValue(kind: LegacyKind, decoded: unknown): unknown {
-	const obj = decoded as Record<string, unknown>
-	switch (kind) {
-		case 'flow':
-			// The legacy bundle wrapped the Flow alongside view-state fields
-			// (selectedId, draft_triggers, ...). The new entry stores only the
-			// Flow — the view-state lives elsewhere or is re-derived.
-			return obj.flow
-		case 'app':
-			// Legacy stored the App directly.
-			return obj
-		case 'raw_app':
-			// Legacy bundle missed the `summary` field that the new editor adds.
-			return {
-				files: obj.files ?? {},
-				runnables: obj.runnables ?? {},
-				data: obj.data ?? {},
-				summary: typeof obj.summary === 'string' ? obj.summary : ''
-			}
-	}
-}
-
-function newKey(workspace: string, kind: LegacyKind, path: string): string {
-	return `userdraft/w/${workspace}/${kind}/${path}`
-}
-
 function listLocalStorageKeys(): string[] {
 	const out: string[] = []
 	for (let i = 0; i < localStorage.length; i++) {
@@ -139,16 +119,11 @@ function listLocalStorageKeys(): string[] {
 }
 
 /**
- * Run the legacy → new-format migration. Idempotent: returns immediately if a
- * previous run completed (signalled by `MIGRATION_FLAG`).
- *
- * The migration is workspace-scoped because the legacy keys had no notion of
- * workspace — we treat the caller's current workspace as the owner of any
- * surviving legacy entries.
+ * Remove the workspace-blind legacy autosave keys (see file header). Idempotent:
+ * returns immediately if a previous run completed (signalled by `MIGRATION_FLAG`).
  */
-export function migrateLegacyUserDrafts(workspace: string): void {
+export function purgeLegacyUserDrafts(): void {
 	if (typeof localStorage === 'undefined') return
-	if (!workspace) return
 	if (localStorage.getItem(MIGRATION_FLAG) !== null) return
 
 	try {
@@ -157,32 +132,18 @@ export function migrateLegacyUserDrafts(workspace: string): void {
 			if (!match) continue
 			const raw = localStorage.getItem(key)
 			if (raw == null) continue
-
-			try {
-				const decoded = decodeLegacyState(raw)
-				if (!isPlausibleLegacyValue(match.newKind, decoded)) continue
-				const value = transformLegacyValue(match.newKind, decoded)
-				const target = newKey(workspace, match.newKind, match.path)
-				if (value !== undefined && localStorage.getItem(target) == null) {
-					// `lastWrittenAt` makes the migrated entry visible to
-					// `gcUserDrafts`. We stamp it as "now" so a freshly-migrated
-					// autosave gets the full retention window — sweeping it
-					// immediately on the first GC pass would lose work the
-					// legacy migration just rescued.
-					localStorage.setItem(target, JSON.stringify({ value, lastWrittenAt: Date.now() }))
-				}
-				localStorage.removeItem(key)
-			} catch (e) {
-				console.error('UserDraft legacy migration: failed to migrate', key, e)
-			}
+			// Only drop keys we can positively recognise as legacy Windmill
+			// drafts; leave unrelated or unparseable look-alikes in place.
+			if (!isPlausibleLegacyValue(match.newKind, decodeLegacyState(raw))) continue
+			localStorage.removeItem(key)
 		}
 		localStorage.setItem(MIGRATION_FLAG, new Date().toISOString())
 	} catch (e) {
-		console.error('UserDraft legacy migration: aborted', e)
+		console.error('UserDraft legacy purge: aborted', e)
 	}
 }
 
-/** Test-only: clear the sentinel so the migration can re-run. */
+/** Test-only: clear the sentinel so the purge can re-run. */
 export function __resetUserDraftLegacyMigrationForTesting(): void {
 	try {
 		localStorage.removeItem(MIGRATION_FLAG)

frontend/src/routes/(root)/(logged)/+layout.svelte

@@ -58,7 +58,7 @@
 	import GlobalSearchModal from '$lib/components/search/GlobalSearchModal.svelte'
 	import MenuButton from '$lib/components/sidebar/MenuButton.svelte'
 	import { loadProtectionRules } from '$lib/workspaceProtectionRules.svelte'
-	import { migrateLegacyUserDrafts } from '$lib/userDraftLegacyMigration'
+	import { purgeLegacyUserDrafts } from '$lib/userDraftLegacyMigration'
 	import { migrateUserDraftsToDb } from '$lib/userDraftDbMigration'
 	import DraftMigrationErrorModal from '$lib/components/DraftMigrationErrorModal.svelte'
 	import { setContext, untrack } from 'svelte'
@@ -435,16 +435,17 @@
 	$effect(() => {
 		$workspaceStore && untrack(() => onLoad())
 	})
-	// One-shot UserDraft migration chain. `migrateLegacyUserDrafts` folds
-	// the legacy `flow` / `app-…` / `rawapp-…` LS keys into the
-	// `userdraft/w/{ws}/{kind}/{path}` format; `migrateUserDraftsToDb`
-	// then pushes those onto the server-side draft table and clears LS
-	// on success. The order matters — the second step only sees what
-	// the first one normalized.
+	// One-shot UserDraft migration. `purgeLegacyUserDrafts` drops the oldest
+	// workspace-blind `flow` / `app-…` / `rawapp-…` LS autosave keys (they
+	// can't be attributed to a workspace, so promoting them would mis-file
+	// drafts). `migrateUserDraftsToDb` then pushes the workspace-scoped
+	// `userdraft/w/{ws}/{kind}/{path}` keys — written by the editor with the
+	// correct workspace — onto the server-side draft table, clearing LS on
+	// success.
 	$effect(() => {
 		if ($workspaceStore && $userStore) {
 			untrack(() => {
-				migrateLegacyUserDrafts($workspaceStore!)
+				purgeLegacyUserDrafts()
 				void migrateUserDraftsToDb()
 			})
 		}