refactor: pin the version this deploy wrote even when one landed on top, and tighten three comment blocks

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

backend/windmill-api-workspaces/src/workspaces.rs

@@ -7316,20 +7316,15 @@ async fn clone_drafts(
     // clone's own principal at deploy time, which is the more accurate answer of the two.
     sqlx::query!(
         // A script hash is content-addressed and copied as-is, so a script draft's base
-        // still names a version the clone has. `clone_flows` / `clone_apps` mint new
-        // `flow_version` / `app_version` ids, so those drafts arrive with no base (their
-        // staleness falls back to the timestamps) rather than one naming a version of the
-        // source workspace, and the lineage field goes with it so the next autosave cannot
-        // re-derive the source id.
+        // still names a version the clone has. `clone_flows` / `clone_apps` mint new ids,
+        // so those drafts arrive with no base (staleness falls back to the timestamps),
+        // lineage field included, or the next autosave would re-derive the source id.
         //
-        // `clean` is `strip_json_nul`'s parity rule in SQL: a draft written before that
-        // sanitizer can carry a U+0000 escape, which `to_jsonb` rejects. Sanitizing on the
-        // way in keeps such a row from either aborting the clone or arriving with its
-        // principal unstripped. Escaped backslashes are parked on chr(1) first, so only an
-        // odd-parity backslash-u0000 (a real NUL) is removed; chr(1) is lossless as a
-        // placeholder because a `json` value's text cannot hold a raw control byte (JSON
-        // escapes them), so nothing in the payload can collide with it. chr(92) spells the
-        // backslash so no escape sequence reaches this source file.
+        // `clean` is `strip_json_nul`'s parity rule in SQL, so a pre-sanitizer U+0000
+        // escape cannot abort the clone on `to_jsonb` or arrive with its principal
+        // unstripped: escaped backslashes park on chr(1) (lossless, a `json` value's text
+        // cannot hold a raw control byte) so only a real NUL is removed, and chr(92)
+        // spells the backslash so no escape sequence reaches this source file.
         r#"INSERT INTO draft (workspace_id, path, typ, value, created_at, email, base)
          SELECT $2, path, typ,
                 to_json(

backend/windmill-common/src/user_drafts.rs

@@ -529,7 +529,6 @@ pub async fn delete_drafts_of_email(conn: &mut sqlx::PgConnection, email: &str)
     Ok(())
 }
 
-
 /// Move the drafts an address owns onto its new address, for the same reason
 /// [`delete_drafts_of_email`] exists: no foreign key follows the rename, so drafts left behind are
 /// stranded on an address that no longer authenticates. Same authorization contract, for a rename.
@@ -651,26 +650,19 @@ pub async fn delete_own_draft_for_path(
 /// string, so without this a move detaches every draft on the item. No owner
 /// filter: teammates' rows and the legacy NULL-email row follow too.
 ///
-/// **The caller must have authorized the underlying item move first.** This
-/// rewrites every owner's row at `old_path`, deliberately including rows the
-/// caller has no permission on, and enforces nothing itself — it takes the paths
-/// on trust. It is safe only because it runs inside a deploy that has already
-/// cleared both paths for the caller; reached any other way it is a cross-user
-/// write with no gate.
+/// **The caller must have authorized the underlying item move first.** This rewrites
+/// rows the caller has no permission on and enforces nothing itself, taking both paths
+/// on trust: it is safe only inside a deploy that has already cleared them, and is a
+/// cross-user write with no gate anywhere else.
 ///
-/// The value keeps its base version: a move is a deploy like any other, so every
-/// draft on the item is now behind the head, which the editor reports through
-/// the ordinary stale-draft prompt. Of its two path keys (`path`, `draft_path`),
-/// one still naming `old_path` is not a rename the user staged — the editors write
-/// the item's own path there on every save — so it follows the row, or deploying
-/// the draft would send the item back where it came from. Any other value is a
-/// staged rename, and is kept.
+/// The value keeps its base version, so every carried draft reads as behind the head the
+/// move minted. Of its two path keys, one still naming `old_path` follows the row (the
+/// editors write the item's own path there on every save, so it is the item's path and
+/// not a staged rename); any other value is a rename the user staged, and is kept.
 ///
-/// A draft already at `new_path` (a never-deployed item, or one left on an
-/// archived script there) occupies that path the way a deployed item does, and
-/// the move is refused with `BadRequest` — inside the deploy's transaction, so
-/// the rename itself is what gets refused. Moving onto it would either merge two
-/// items or strand the row that lost the collision.
+/// A draft already at `new_path` occupies it the way a deployed item does, so the move is
+/// refused with `BadRequest` inside the deploy's transaction, refusing the rename itself:
+/// moving onto it would merge two items or strand the row that lost.
 pub async fn move_drafts_for_path(
     tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,
     w_id: &str,
@@ -760,11 +752,10 @@ pub async fn move_drafts_for_path(
 /// draft write at `old_path` (any owner's, for an item move), and enforces nothing
 /// itself.
 ///
-/// A record outlives the editors that need it: it ends when a later move touches
-/// either path or an item is deployed at `old_path`, so a save that means to start
-/// a NEW draft at a vacated path would be routed instead. Nothing does that today —
-/// every surface parks a new item at a minted `u/<user>/draft_<uuid>` key
-/// (`mintDraftPath.ts`) and carries the user-typed name inside the value.
+/// A record outlives the editors that need it: it ends when a later move touches either
+/// path or an item is deployed at `old_path`, so a save meaning to start a NEW draft at a
+/// vacated path would be routed instead. Nothing does that today: every surface parks a
+/// new item at a minted `u/<user>/draft_<uuid>` key (`mintDraftPath.ts`).
 pub async fn record_draft_move(
     tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,
     w_id: &str,

frontend/src/lib/components/DiffDrawer.svelte

@@ -85,10 +85,9 @@
 	}
 
 	/** Counted per opening, and counted here rather than in the editor that opens one: a
-	 *  path change remounts the editor while this drawer stays mounted, so a counter local
-	 *  to the editor is one an outlived request still matches, and it would open and fill
-	 *  the drawer with the item the user just left. Every write an opening makes (the
-	 *  blanking `openDrawer` included) checks `ownsOpening` first. */
+	 *  path change remounts the editor while this drawer stays, so a counter local to it is
+	 *  one an outlived request still matches, and that request would fill the drawer with
+	 *  the item the user just left. Every write an opening makes checks this first. */
 	let openingToken = 0
 
 	export function beginOpening(): number {
@@ -99,11 +98,10 @@
 		return token === openingToken
 	}
 
-	/** Drop an opening and everything it put on screen: the editor that started it is going
-	 *  away (a path change remounts it), so a diff it already filled describes an item this
-	 *  drawer can no longer act on — Take latest would move the base of whatever loaded in
-	 *  its place — and one still in flight would leave the spinner behind. A no-op once
-	 *  another opening owns the drawer: what it shows is then someone else's. */
+	/** Drop an opening and everything it put on screen: its editor is going away, so a diff
+	 *  it filled acts on an item that is gone (Take latest would move the base of whatever
+	 *  loaded in its place) and one still in flight would leave the spinner behind. A no-op
+	 *  once another opening owns the drawer: what it shows is then someone else's. */
 	export function abandonOpening(token: number) {
 		if (token !== openingToken) return
 		openingToken++

frontend/src/lib/components/apps/editor/AppEditorHeader.svelte

@@ -393,7 +393,7 @@
 			path: npath
 		})
 		version = versionThisDeployWrote(appHistory, $userStore?.username)
-		// Re-pin the fork base to the just-deployed head: the editor stays open, so a
+		// Re-pin the fork base to the version just written: the editor stays open, so a
 		// follow-up deploy (or a new edit) would otherwise compare against the now-
 		// superseded base and falsely warn. parent_version is in
 		// DRAFT_COMPARE_IGNORED_FIELDS, so this write can't spawn a spurious draft.

frontend/src/lib/components/apps/editor/appDeploy.svelte.ts

@@ -7,22 +7,15 @@ export function computeSecretUrl(secretUrl: string) {
 }
 
 /**
- * The version a just-finished deploy wrote, read back from the history it lands in.
- * The deploy and this read are two requests, so someone else's deploy in between is the
- * newest entry too; taking it would make their version the fork base of content it never
- * contained, and the next deploy would find base === head and overwrite them with no
- * warning. A head this caller did not write therefore yields `undefined`: an unknown
- * base, which the timestamps still cover, rather than a wrong one.
- *
- * Author is all the history can be matched on (`deployment_msg` is written later, by the
- * dependency job), so a second deploy by the same user inside that window still reads as
- * this one's.
+ * The version a just-finished deploy wrote: this caller's newest history entry, which the
+ * awaited deploy guarantees is the one it wrote. Taking the head instead would pin a
+ * deploy that landed in between as the base of content it never contained, and the next
+ * deploy would then find base === head and overwrite it with no warning.
  */
 export function versionThisDeployWrote(
 	history: { version: number; created_by?: string }[] | undefined,
 	deployedBy: string | undefined
 ): number | undefined {
-	const head = history?.[0]
-	if (!head || !deployedBy || head.created_by !== deployedBy) return undefined
-	return head.version
+	if (!deployedBy) return undefined
+	return history?.find((h) => h.created_by === deployedBy)?.version
 }

frontend/src/lib/components/apps/editor/appDeploy.test.ts

@@ -6,9 +6,21 @@ describe('versionThisDeployWrote', () => {
 		expect(versionThisDeployWrote([{ version: 7, created_by: 'alice' }], 'alice')).toBe(7)
 	})
 
-	it('claims nothing when another deploy landed on top', () => {
-		// The base would otherwise be a version this content never contained, and the
-		// next deploy would find base === head and overwrite it unwarned.
+	it('finds its own version under a deploy that landed on top', () => {
+		// Pinning the head would make Bob's version the base of content it never
+		// contained, and the next deploy would find base === head and overwrite it.
+		expect(
+			versionThisDeployWrote(
+				[
+					{ version: 8, created_by: 'bob' },
+					{ version: 7, created_by: 'alice' }
+				],
+				'alice'
+			)
+		).toBe(7)
+	})
+
+	it('claims nothing it cannot attribute', () => {
 		expect(versionThisDeployWrote([{ version: 8, created_by: 'bob' }], 'alice')).toBe(undefined)
 		expect(versionThisDeployWrote([], 'alice')).toBe(undefined)
 		expect(versionThisDeployWrote([{ version: 7, created_by: 'alice' }], undefined)).toBe(undefined)

frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte

@@ -545,7 +545,8 @@
 			workspace: opWorkspace!,
 			path: npath
 		})
-		// Only a version this deploy can claim becomes the draft's base below.
+		// The version this deploy wrote, not the head: they differ when someone else's
+		// deploy landed in between, and this becomes the next draft's base below.
 		version = versionThisDeployWrote(appHistory, $userStore?.username)
 
 		closeSaveDrawer()