From ee2c2abcb9dbeae55d9bddc8df97724e0d595633 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Mon, 14 Sep 2026 20:15:04 +0200 Subject: [PATCH] fix: session loaders keep a draft's base, and a failed relocation flush stays put Co-Authored-By: Claude Opus 5 (1M context) --- backend/tests/drafts_save_follows_move.rs | 82 +++++++++++++++++++ backend/windmill-common/src/user_drafts.rs | 13 +++ .../DraftEditorModals.svelte | 10 +++ .../sessions/sessionRuntime.svelte.ts | 21 +++-- 4 files changed, 121 insertions(+), 5 deletions(-) diff --git a/backend/tests/drafts_save_follows_move.rs b/backend/tests/drafts_save_follows_move.rs index 820a312117..79683b7d75 100644 --- a/backend/tests/drafts_save_follows_move.rs +++ b/backend/tests/drafts_save_follows_move.rs @@ -157,3 +157,85 @@ async fn test_save_follows_a_draft_only_move(db: Pool) -> anyhow::Resu assert_eq!(draft["draft_path"], "u/test-user/moved", "{draft}"); Ok(()) } + +/// Rename `from` to `to` the way Home does: redeploy the deployed content at the new +/// path, keeping the deployer's own draft so it is carried rather than consumed. +/// Returns the new head's hash. +async fn rename(port: u16, from_hash: &str, to: &str) -> anyhow::Result { + let resp = reqwest::Client::new() + .post(format!( + "http://localhost:{port}/api/w/test-workspace/scripts/create" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .json(&json!({ + "path": to, + "parent_hash": from_hash, + "summary": "A", + "description": "", + "content": "export function main() { return 1 }", + "language": "deno", + "schema": {}, + "skip_draft_deletion": true + })) + .send() + .await?; + let status = resp.status(); + let hash = resp.text().await?; + assert_eq!(status, 201, "rename to {to} failed: {hash}"); + Ok(hash) +} + +/// Save the draft as an editor still bound to `url_path` would. Returns the path the +/// save landed at. +async fn save_at(port: u16, url_path: &str, content: &str) -> anyhow::Result { + let saved: Value = reqwest::Client::new() + .post(format!( + "http://localhost:{port}/api/w/test-workspace/drafts/update/script/{url_path}" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .json(&json!({ + "value": { "path": url_path, "summary": "A", "content": content, "language": "deno" } + })) + .send() + .await? + .json() + .await?; + assert_eq!(saved["status"], "saved", "save refused: {saved}"); + Ok(saved["path"].as_str().unwrap_or_default().to_string()) +} + +/// A record is kept to one hop, and a move back to the path it left ends it: both are +/// three statements whose order decides the answer. +#[sqlx::test(fixtures("base", "drafts_save_follows_move"))] +async fn test_move_records_stay_one_hop(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + let b = rename(port, HEAD_HASH, "u/test-user/follow_b").await?; + let _c = rename(port, &b, "u/test-user/follow_c").await?; + assert_eq!( + save_at(port, "u/test-user/follow_a", "after two moves").await?, + "u/test-user/follow_c", + "a save at the first path did not reach the last" + ); + assert_eq!(own_draft_paths(port).await?, vec!["u/test-user/follow_c"]); + Ok(()) +} + +#[sqlx::test(fixtures("base", "drafts_save_follows_move"))] +async fn test_move_back_ends_the_record(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + let b = rename(port, HEAD_HASH, "u/test-user/follow_b").await?; + let _a = rename(port, &b, "u/test-user/follow_a").await?; + assert_eq!( + save_at(port, "u/test-user/follow_a", "after moving back").await?, + "u/test-user/follow_a", + "a save was routed off the path the item moved back to" + ); + assert_eq!(own_draft_paths(port).await?, vec!["u/test-user/follow_a"]); + Ok(()) +} diff --git a/backend/windmill-common/src/user_drafts.rs b/backend/windmill-common/src/user_drafts.rs index 6be8d4b7fe..6b9f74e7c4 100644 --- a/backend/windmill-common/src/user_drafts.rs +++ b/backend/windmill-common/src/user_drafts.rs @@ -722,6 +722,16 @@ pub async fn move_drafts_for_path( /// /// Kept to one hop: records pointing at `old_path` are re-pointed, and records /// leaving either path are replaced, since `new_path` now holds the item. +/// +/// **The caller must have authorized the move first.** A record routes every later +/// 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//draft_` key +/// (`mintDraftPath.ts`) and carries the user-typed name inside the value. pub async fn record_draft_move( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, w_id: &str, @@ -771,6 +781,9 @@ pub async fn record_draft_move( /// Drop the move records leaving `path`: an item was just created there, and saves /// addressed to it are its own. +/// +/// **The caller must have authorized the deploy that created the item first.** +/// Dropping a record sends later draft writes at `path` back to `path`. pub async fn clear_draft_moves_from( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, w_id: &str, diff --git a/frontend/src/lib/components/common/confirmationModal/DraftEditorModals.svelte b/frontend/src/lib/components/common/confirmationModal/DraftEditorModals.svelte index d6463f2cd9..3b62b07101 100644 --- a/frontend/src/lib/components/common/confirmationModal/DraftEditorModals.svelte +++ b/frontend/src/lib/components/common/confirmationModal/DraftEditorModals.svelte @@ -148,6 +148,16 @@ relocating = true await onBeforeRelocate?.() await UserDraftDbSyncer.flush(query) + // `flush` resolves on a failed or rejected save as well, and leaving the + // route drops what it was carrying: stay, so the editor keeps the edits + // and its own failure indicator. + if ( + UserDraftDbSyncer.getState(query).failureMessage || + UserDraftDbSyncer.getConflict(query).conflict + ) { + relocating = false + return + } sendUserToast(`This item was moved to ${newPath}. You are now editing it there.`) await goto(`${base}/${seg}/${newPath}`) }) diff --git a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts index 1b1fc16834..eb2ecff6dd 100644 --- a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts @@ -607,7 +607,9 @@ function createRuntime(session: Session): SessionRuntime { saved.val = undefined } await initFlow(aiDraft, store, stateStore, workspace) - if (deployedVersionId != null && store.val) store.val.version_id = deployedVersionId + // Only a draft with no base takes the head; see loadScript. + if (deployedVersionId != null && store.val && store.val.version_id == null) + store.val.version_id = deployedVersionId slot.loadedPath = path slot.loadedWorkspace = workspace return @@ -629,7 +631,8 @@ function createRuntime(session: Session): SessionRuntime { ) UserDraft.save('flow', path, flow, { workspace }) await initFlow(flow, store, stateStore, workspace) - if (deployedVersionId != null && store.val) store.val.version_id = deployedVersionId + if (deployedVersionId != null && store.val && store.val.version_id == null) + store.val.version_id = deployedVersionId slot.loadedPath = path slot.loadedWorkspace = workspace } catch (err) { @@ -694,7 +697,10 @@ function createRuntime(session: Session): SessionRuntime { schema: emptySchema(), language: (aiDraft.language ?? 'bun') as any } - if (saved.val?.hash) { + // Only a draft with no base takes the head: `draft.base` is derived from + // `parent_hash` on every save, so stamping the head over a fork base + // tells the server this draft is up to date when it is not. + if (saved.val?.hash && baseline.parent_hash == null) { baseline.parent_hash = saved.val.hash } baseline.content = aiDraft.content @@ -714,7 +720,8 @@ function createRuntime(session: Session): SessionRuntime { const baseline = structuredClone( ((result as SavedScript).draft as NewScript | undefined) ?? (result as NewScript) ) - baseline.parent_hash = result.hash + // See the ai-draft branch above: the head only seeds a draft that has no base. + if (baseline.parent_hash == null) baseline.parent_hash = result.hash // Seed the per-tab last_sync from the server draft's timestamp so the // seeding save below attaches a matching last_sync and the server can // reject stale writes (see loadRawApp). Without this a server draft — @@ -842,7 +849,11 @@ function createRuntime(session: Session): SessionRuntime { path: result.path, custom_path: draftValue?.custom_path ?? result.custom_path, draft_path: draftValue?.draft_path, - parent_version: draftValue?.parent_version + // The draft's own base, else the head this checkout forks from (the + // standalone editor's loader does the same). + parent_version: + draftValue?.parent_version ?? + (Array.isArray(result.versions) ? result.versions[result.versions.length - 1] : undefined) } // Seed the per-tab last_sync from the server draft's timestamp so // later saves attach a matching last_sync and the server can reject