From 3301d81ed6c4acf7acf0465d47acbaafb50e2172 Mon Sep 17 00:00:00 2001
From: Diego Imbert <diego@windmill.dev>
Date: Wed, 13 May 2026 15:40:17 +0200
Subject: [PATCH] refactor(frontend): per-workspace UserDraft handles in
 Resource/Variable editors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Earlier commits in this PR wired the resource and variable editors to a
single multi-workspace bundle stored under the user's session workspace
key — which mixed workspaces in one localStorage entry and required a
custom multi-key fix-up pass to persist edits for other workspaces.

Reset both editors to their pre-PR shape and apply the minimal change:
the per-workspace `Record<string, ResourceState>` (resp. `VariableState`)
becomes `Record<string, UserDraftHandle<…>>`, with one handle per
workspace created via `UserDraft.use(…, { workspace: ws })`. The handle
keys its own localStorage entry under that workspace, so cross-workspace
edits stay cleanly separated and reactivity flows through the handle's
`draft` accessor — `bind:` on form fields just works.

Adds `manualRelease: true` + `handle.release()` to `UserDraft.use` so
the editors can register handles lazily inside an effect (Svelte 5
forbids `onDestroy` outside component init). The editors register a
single top-level `onDestroy` that releases every collected handle.

After a successful save, the per-workspace autosave is cleared via
`UserDraft.remove(itemKind, path, { workspace })`.
---
 .../src/lib/components/ResourceEditor.svelte  | 105 ++++++++----------
 .../src/lib/components/VariableEditor.svelte  |  89 +++++++--------
 frontend/src/lib/userDraft.svelte.ts          |  28 ++++-
 frontend/src/lib/userDraft.test.ts            |  22 ++++
 4 files changed, 135 insertions(+), 109 deletions(-)

diff --git a/frontend/src/lib/components/ResourceEditor.svelte b/frontend/src/lib/components/ResourceEditor.svelte
index 4e8d2cb363..39bf946f21 100644
--- a/frontend/src/lib/components/ResourceEditor.svelte
+++ b/frontend/src/lib/components/ResourceEditor.svelte
@@ -1,8 +1,8 @@
 <script lang="ts">
 	import type { Schema } from '$lib/common'
 	import { ResourceService, WorkspaceService, type Resource, type ResourceType } from '$lib/gen'
-	import { canWrite, readFieldsRecursively } from '$lib/utils'
-	import { createEventDispatcher, untrack } from 'svelte'
+	import { canWrite } from '$lib/utils'
+	import { createEventDispatcher, onDestroy, untrack } from 'svelte'
 	import { userStore, workspaceStore } from '$lib/stores'
 	import { sendUserToast } from '$lib/toast'
 	import { clearJsonSchemaResourceCache } from './schema/jsonSchemaResource.svelte'
@@ -12,7 +12,7 @@
 	import { deepEqual } from 'fast-equals'
 	import { getUserExt } from '$lib/user'
 	import type { UserExt } from '$lib/stores'
-	import { UserDraft } from '$lib/userDraft.svelte'
+	import { UserDraft, type UserDraftHandle } from '$lib/userDraft.svelte'
 
 	interface Props {
 		canSave?: boolean
@@ -44,25 +44,40 @@
 		wsSpecific: boolean
 	}
 
-	// The local autosave is the entire `states` map, keyed by target workspace.
-	// This editor can edit several workspaces in one session (cross-workspace
-	// deploys); persisting only the current workspace would silently drop
-	// edits made on the other tabs.
-	type ResourceDraft = Record<string, ResourceState>
-
 	const dispatch = createEventDispatcher()
 
 	let effectiveWorkspace = $derived(workspace ?? $workspaceStore!)
 	let initialPath = path
 
-	const resourceDraftHandle = UserDraft.use<ResourceDraft>('resource', initialPath ?? '')
-
-	let states: Record<string, ResourceState> = $state({})
+	// Per-workspace handles. Each workspace's autosave lives at its own
+	// localStorage key (`userdraft/w/{ws}/resource/{initialPath}`) so editing
+	// the same path across two workspaces stays cleanly separated.
+	let states: Record<string, UserDraftHandle<ResourceState>> = $state({})
 	let initialStates: Record<string, ResourceState> = $state({})
 	let existedInitially: Record<string, boolean> = $state({})
 	let fetchedResources: Record<string, Resource> = $state({})
 	let perWsUser: Record<string, UserExt | undefined> = $state({})
 
+	onDestroy(() => {
+		for (const h of Object.values(states)) h.release()
+	})
+
+	/** Create (or reuse) a per-workspace handle, seeding it with `baseline` if
+	 * no local autosave is already present. */
+	function ensureHandle(ws: string, baseline: ResourceState): UserDraftHandle<ResourceState> {
+		if (states[ws]) return states[ws]
+		const h = UserDraft.use<ResourceState>('resource', initialPath ?? '', {
+			workspace: ws,
+			manualRelease: true
+		})
+		// Existing autosave wins; only seed when there's nothing persisted yet.
+		// The seed itself doesn't persist (saveInitialValue=false) — only the
+		// user's first real edit triggers a write.
+		if (h.draft === undefined) h.draft = baseline
+		states[ws] = h
+		return h
+	}
+
 	let isValid = $state(true)
 	let jsonError = $state('')
 	let viewJsonSchema = $state(false)
@@ -94,7 +109,7 @@
 	})
 	let loadingSchema = $derived(resourceTypeResource.loading)
 
-	let current = $derived(selected ? states[selected] : undefined)
+	let current = $derived(selected ? states[selected]?.draft : undefined)
 	let resourceToEdit: Resource | undefined = $derived(
 		selected ? fetchedResources[selected] : undefined
 	)
@@ -116,7 +131,7 @@
 	)
 
 	const dirtyWorkspaces = $derived(
-		Object.keys(states).filter((ws) => !deepEqual(states[ws], initialStates[ws]))
+		Object.keys(states).filter((ws) => !deepEqual(states[ws].draft, initialStates[ws]))
 	)
 	const anyDirty = $derived(dirtyWorkspaces.length > 0)
 	const otherDirty = $derived(
@@ -130,25 +145,24 @@
 			const r = fetchedResources[ws]
 			return (
 				!r ||
-				canWrite(states[ws]?.path ?? initialPath, r.extra_perms ?? {}, perWsUser[ws] ?? $userStore)
+				canWrite(
+					states[ws]?.draft?.path ?? initialPath,
+					r.extra_perms ?? {},
+					perWsUser[ws] ?? $userStore
+				)
 			)
 		})
 	)
 
-	// Bootstrap: ensure selected is set on mount (edit or new). For new
-	// resources we also rehydrate from a local autosave (UserDraft) so
-	// returning to the drawer mid-edit keeps your work — including edits
-	// staged for additional target workspaces.
+	// Bootstrap: ensure selected is set on mount (edit or new)
 	$effect(() => {
 		if (selected !== undefined) return
 		if (!effectiveWorkspace) return
 		untrack(() => {
 			selected = effectiveWorkspace
 			if (!initialPath) {
-				const localBundle = resourceDraftHandle.draft
-				const localFor = (ws: string): ResourceState | undefined => localBundle?.[ws]
-
-				const baseState: ResourceState = {
+				// New resource
+				const s: ResourceState = {
 					path: '',
 					description: '',
 					args: (defaultValues && Object.keys(defaultValues).length > 0
@@ -157,23 +171,9 @@
 					labels: undefined,
 					wsSpecific: false
 				}
-
-				const local = localFor(effectiveWorkspace)
-				states[effectiveWorkspace] = local ? structuredClone(local) : baseState
-				// For new resources the "initial state" is the pristine empty
-				// state — that's the baseline dirty is computed against.
-				initialStates[effectiveWorkspace] = structuredClone(baseState)
+				ensureHandle(effectiveWorkspace, s)
+				initialStates[effectiveWorkspace] = structuredClone(s)
 				existedInitially[effectiveWorkspace] = false
-
-				// Restore any other workspaces the user had staged edits for.
-				if (localBundle) {
-					for (const ws of Object.keys(localBundle)) {
-						if (ws === effectiveWorkspace) continue
-						states[ws] = structuredClone(localBundle[ws])
-						initialStates[ws] = structuredClone(baseState)
-						existedInitially[ws] = false
-					}
-				}
 			}
 		})
 	})
@@ -189,22 +189,15 @@
 				getUserExt(ws)
 			]).then(([r, user]) => {
 				fetchedResources[ws] = r
-				const backendState: ResourceState = {
+				const s: ResourceState = {
 					path: r.path,
 					description: r.description ?? '',
 					args: (r.value ?? {}) as any,
 					labels: r.labels ?? undefined,
 					wsSpecific: r.ws_specific ?? false
 				}
-				// If the local autosave has a saved state for *this* workspace
-				// and it diverges from the backend, use the local one. The
-				// localStorage entry itself lives under the user's session
-				// workspace (UserDraft's key) regardless of which target
-				// workspace this state belongs to.
-				const localState = resourceDraftHandle.draft?.[ws]
-				const useLocal = localState && !deepEqual(localState, backendState)
-				states[ws] = useLocal ? structuredClone(localState) : backendState
-				initialStates[ws] = structuredClone(backendState)
+				ensureHandle(ws, s)
+				initialStates[ws] = structuredClone(s)
 				existedInitially[ws] = true
 				perWsUser[ws] = user
 				// Keep resource_type in sync for the base workspace (controls the schema)
@@ -215,13 +208,6 @@
 		})
 	})
 
-	// Auto-persist the full multi-workspace edit bundle on every mutation.
-	// useLocalStorageValue's lastSerialized check dedupes writes.
-	$effect(() => {
-		readFieldsRecursively(states)
-		resourceDraftHandle.draft = states
-	})
-
 	// Keep current.path bound to the outer `path` prop for consumers
 	$effect(() => {
 		if (current) path = current.path
@@ -257,7 +243,7 @@
 		const dirty = dirtyWorkspaces
 		try {
 			for (const ws of dirty) {
-				const s = states[ws]
+				const s = states[ws].draft!
 				const ini = initialStates[ws]
 				if (existedInitially[ws]) {
 					await ResourceService.updateResource({
@@ -288,11 +274,14 @@
 						}
 					})
 				}
+				// Saved on the backend — drop the local autosave for this
+				// workspace and refresh the dirty baseline.
+				initialStates[ws] = structuredClone(s)
+				UserDraft.remove('resource', initialPath ?? '', { workspace: ws })
 			}
 			sendUserToast(
 				dirty.length > 1 ? `Saved resource in ${dirty.length} workspaces` : `Saved resource`
 			)
-			UserDraft.remove('resource', initialPath ?? '')
 			dispatch('refresh', current?.path ?? path)
 		} catch (err) {
 			sendUserToast(`Could not save resource: ${err.body ?? err.message}`, true)
diff --git a/frontend/src/lib/components/VariableEditor.svelte b/frontend/src/lib/components/VariableEditor.svelte
index 1700ae3650..6737752514 100644
--- a/frontend/src/lib/components/VariableEditor.svelte
+++ b/frontend/src/lib/components/VariableEditor.svelte
@@ -1,13 +1,13 @@
 <script lang="ts">
 	import { VariableService, WorkspaceService } from '$lib/gen'
-	import { createEventDispatcher, untrack } from 'svelte'
+	import { createEventDispatcher, onDestroy, untrack } from 'svelte'
 	import { userStore, workspaceStore } from '$lib/stores'
 	import { Button } from './common'
 	import Drawer from './common/drawer/Drawer.svelte'
 	import DrawerContent from './common/drawer/DrawerContent.svelte'
 	import Alert from './common/alert/Alert.svelte'
 	import { sendUserToast } from '$lib/toast'
-	import { canWrite, readFieldsRecursively } from '$lib/utils'
+	import { canWrite } from '$lib/utils'
 	import { Save } from 'lucide-svelte'
 	import VariableForm from './VariableForm.svelte'
 	import WsSpecificVersions from './WsSpecificVersions.svelte'
@@ -15,7 +15,7 @@
 	import { deepEqual } from 'fast-equals'
 	import { getUserExt } from '$lib/user'
 	import type { UserExt } from '$lib/stores'
-	import { UserDraft } from '$lib/userDraft.svelte'
+	import { UserDraft, type UserDraftHandle } from '$lib/userDraft.svelte'
 
 	const dispatch = createEventDispatcher()
 
@@ -28,7 +28,10 @@
 
 	let editPath: string | undefined = $state(undefined)
 
-	let states: Record<string, VariableState> = $state({})
+	// Per-workspace handles. Each workspace's autosave lives at its own
+	// localStorage key (`userdraft/w/{ws}/variable/{editPath}`) so editing the
+	// same path across two workspaces stays cleanly separated.
+	let states: Record<string, UserDraftHandle<VariableState>> = $state({})
 	let initialStates: Record<string, VariableState> = $state({})
 	let existedInitially: Record<string, boolean> = $state({})
 	let extraPerms: Record<string, Record<string, boolean>> = $state({})
@@ -36,6 +39,23 @@
 	let selected: string | undefined = $state(undefined)
 	let pathError = $state('')
 
+	onDestroy(() => {
+		for (const h of Object.values(states)) h.release()
+	})
+
+	/** Create (or reuse) a per-workspace handle, seeding with `baseline` when
+	 * no autosave is already persisted. */
+	function ensureHandle(ws: string, baseline: VariableState): UserDraftHandle<VariableState> {
+		if (states[ws]) return states[ws]
+		const h = UserDraft.use<VariableState>('variable', editPath ?? '', {
+			workspace: ws,
+			manualRelease: true
+		})
+		if (h.draft === undefined) h.draft = baseline
+		states[ws] = h
+		return h
+	}
+
 	let drawer: Drawer | undefined = $state()
 	let form: VariableForm | undefined = $state()
 
@@ -48,7 +68,7 @@
 	const MAX_VARIABLE_LENGTH = 10000
 	const edit = $derived(editPath !== undefined)
 	const initialPath = $derived(editPath ?? '')
-	const current = $derived(selected ? states[selected] : undefined)
+	const current = $derived(selected ? states[selected]?.draft : undefined)
 	const can_write = $derived.by(() => {
 		if (!selected || !edit) return true
 		const perms = extraPerms[selected]
@@ -56,7 +76,7 @@
 		return canWrite(editPath ?? '', perms, perWsUser[selected] ?? $userStore)
 	})
 	const dirtyWorkspaces = $derived(
-		Object.keys(states).filter((ws) => !deepEqual(states[ws], initialStates[ws]))
+		Object.keys(states).filter((ws) => !deepEqual(states[ws].draft, initialStates[ws]))
 	)
 	const anyDirty = $derived(dirtyWorkspaces.length > 0)
 	const otherDirty = $derived(
@@ -65,7 +85,10 @@
 			: dirtyWorkspaces
 	)
 	const dirtyValid = $derived(
-		dirtyWorkspaces.every((ws) => states[ws].variable.value.length <= MAX_VARIABLE_LENGTH)
+		dirtyWorkspaces.every((ws) => {
+			const v = states[ws].draft
+			return !!v && v.variable.value.length <= MAX_VARIABLE_LENGTH
+		})
 	)
 	const dirtyCanWrite = $derived(
 		dirtyWorkspaces.every((ws) => {
@@ -74,12 +97,6 @@
 		})
 	)
 
-	// The local autosave is the entire multi-workspace `states` bundle, so
-	// cross-workspace edits in the same drawer session are preserved across
-	// refresh. UserDraft keys on the user's session workspace regardless of
-	// which target workspace each state entry belongs to.
-	type VariableDraft = Record<string, VariableState>
-
 	// Lazy-fetch the variable for the selected workspace when not already cached
 	$effect(() => {
 		const ws = selected
@@ -91,7 +108,7 @@
 				VariableService.getVariable({ workspace: ws, path: p, decryptSecret: false }),
 				getUserExt(ws)
 			]).then(([v, user]) => {
-				const backendState: VariableState = {
+				const s: VariableState = {
 					path: v.path,
 					variable: {
 						value: v.value ?? '',
@@ -101,11 +118,8 @@
 					labels: v.labels ?? undefined,
 					wsSpecific: v.ws_specific ?? false
 				}
-				const localBundle = UserDraft.get<VariableDraft>('variable', p)
-				const localState = localBundle?.[ws]
-				const useLocal = localState && !deepEqual(localState, backendState)
-				states[ws] = useLocal ? structuredClone(localState) : backendState
-				initialStates[ws] = structuredClone(backendState)
+				ensureHandle(ws, s)
+				initialStates[ws] = structuredClone(s)
 				existedInitially[ws] = true
 				extraPerms[ws] = v.extra_perms ?? {}
 				perWsUser[ws] = user
@@ -113,15 +127,8 @@
 		})
 	})
 
-	// Persist the full states bundle on every mutation. Empty path (new
-	// variable) stays in-memory only; UserDraft.save no-ops there.
-	$effect(() => {
-		if (Object.keys(states).length === 0) return
-		readFieldsRecursively(states)
-		UserDraft.save('variable', editPath ?? '', states)
-	})
-
 	function reset() {
+		for (const h of Object.values(states)) h.release()
 		states = {}
 		initialStates = {}
 		existedInitially = {}
@@ -134,30 +141,15 @@
 		reset()
 		editPath = undefined
 		const ws = $workspaceStore!
-		// Empty-path drafts live in-memory only; rehydrate any in-memory
-		// bundle if one already exists on this page, including states
-		// staged for other target workspaces.
-		const localBundle = UserDraft.get<VariableDraft>('variable', '')
-		const baseState: VariableState = {
+		const s: VariableState = {
 			path: '',
 			variable: { value: '', is_secret: true, description: '' },
 			labels: undefined,
 			wsSpecific: false
 		}
-		const localFor = (w: string): VariableState | undefined => localBundle?.[w]
-
-		const localOwn = localFor(ws)
-		states[ws] = localOwn ? structuredClone(localOwn) : baseState
-		initialStates[ws] = structuredClone(baseState)
+		ensureHandle(ws, s)
+		initialStates[ws] = structuredClone(s)
 		existedInitially[ws] = false
-		if (localBundle) {
-			for (const w of Object.keys(localBundle)) {
-				if (w === ws) continue
-				states[w] = structuredClone(localBundle[w])
-				initialStates[w] = structuredClone(baseState)
-				existedInitially[w] = false
-			}
-		}
 		selected = ws
 		drawer?.openDrawer()
 	}
@@ -176,7 +168,7 @@
 			path: editPath,
 			decryptSecret: true
 		})
-		const s = states[selected]
+		const s = states[selected]?.draft
 		const ini = initialStates[selected]
 		if (s) s.variable.value = getV.value ?? ''
 		if (ini) ini.variable.value = getV.value ?? ''
@@ -187,7 +179,7 @@
 		const dirty = dirtyWorkspaces
 		try {
 			for (const ws of dirty) {
-				const s = states[ws]
+				const s = states[ws].draft!
 				const ini = initialStates[ws]
 				if (existedInitially[ws]) {
 					await VariableService.updateVariable({
@@ -219,9 +211,10 @@
 						}
 					})
 				}
+				// Saved on the backend — drop the local autosave for this workspace.
+				UserDraft.remove('variable', editPath ?? '', { workspace: ws })
 			}
 			sendUserToast(edit ? `Updated variable in ${dirty.length} workspace(s)` : `Created variable`)
-			UserDraft.remove('variable', editPath ?? '')
 			dispatch('create')
 			drawer?.closeDrawer()
 		} catch (err) {
diff --git a/frontend/src/lib/userDraft.svelte.ts b/frontend/src/lib/userDraft.svelte.ts
index cf63db2b17..5d2147cd2c 100644
--- a/frontend/src/lib/userDraft.svelte.ts
+++ b/frontend/src/lib/userDraft.svelte.ts
@@ -40,6 +40,14 @@ export type UserDraftUseOptions<V> = UserDraftOptions & {
 	 * actual mutation is what writes to localStorage.
 	 */
 	defaultValue?: V
+	/**
+	 * When `true`, skip the automatic `onDestroy` registration. The caller is
+	 * responsible for invoking `handle.release()` to decrement the entry's
+	 * refcount. Useful when handles are created dynamically (e.g. inside an
+	 * effect) where `onDestroy` can no longer be called — Svelte 5 only
+	 * accepts lifecycle hooks during component initialization.
+	 */
+	manualRelease?: boolean
 }
 
 /**
@@ -230,6 +238,12 @@ export type UserDraftHandle<V> = {
 	 * just acknowledged it.
 	 */
 	setMeta(meta: UserDraftMeta, opts?: { force?: boolean }): void
+	/**
+	 * Manually decrement the entry's refcount. Only required when `use()` was
+	 * called with `manualRelease: true` — otherwise an `onDestroy` is already
+	 * wired in. Calling this more than once for the same handle is a no-op.
+	 */
+	release(): void
 }
 
 export const UserDraft = {
@@ -371,14 +385,21 @@ export const UserDraft = {
 
 		const sharedEntry = entry
 
-		onDestroy(() => {
+		let released = false
+		const release = (): void => {
+			if (released) return
+			released = true
 			const e = entries.get(mk)
 			if (!e) return
 			e.count--
 			if (e.count <= 0) {
 				entries.delete(mk)
 			}
-		})
+		}
+
+		if (!opts?.manualRelease) {
+			onDestroy(release)
+		}
 
 		return {
 			get draft(): V | undefined {
@@ -405,7 +426,8 @@ export const UserDraft = {
 				if (opts?.force && !isLocalOnly(path)) {
 					persistDirect(localStorageKey(ws, itemKind, path), current.value, meta)
 				}
-			}
+			},
+			release
 		}
 	}
 }
diff --git a/frontend/src/lib/userDraft.test.ts b/frontend/src/lib/userDraft.test.ts
index f897c35f87..574954bf23 100644
--- a/frontend/src/lib/userDraft.test.ts
+++ b/frontend/src/lib/userDraft.test.ts
@@ -471,4 +471,26 @@ describe('UserDraft.use() — reference counting & cleanup', () => {
 		const b = UserDraft.use<string>('flow', 'u/me/cycle')
 		expect(b.draft).toBe('edited')
 	})
+
+	it('manualRelease=true skips onDestroy registration and gates cleanup behind handle.release()', () => {
+		// Caller opts out of the auto-onDestroy — used by routes that create
+		// handles dynamically (e.g. ResourceEditor's per-workspace handles).
+		const h = UserDraft.use<string>('flow', 'u/me/manual', { manualRelease: true })
+		expect(onDestroyCallbacks.length).toBe(0)
+		h.draft = 'initial' // baseline
+		h.draft = 'edited' // persisted
+
+		// Without release(), the entry stays alive — a co-resident handle
+		// sees the same in-memory state.
+		const h2 = UserDraft.use<string>('flow', 'u/me/manual', { manualRelease: true })
+		expect(h2.draft).toBe('edited')
+		h.release()
+		// h2 still holds the entry. Releasing both clears it.
+		UserDraft.save('flow', 'u/me/manual', 'edited2')
+		expect(h2.draft).toBe('edited2')
+
+		h2.release()
+		// Second release on the same handle is a no-op — refcount stays at 0.
+		h2.release()
+	})
 })