From 29e75d27d5e1fec01728593a41f355e76954ef11 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Mon, 14 Sep 2026 22:32:28 +0200 Subject: [PATCH] fix: an unknown base stays unknown in every loader, and an owner move extends an item move Co-Authored-By: Claude Opus 5 (1M context) --- ...1ccdb37e078a7473e554f15b9c1f2ef6cdb95.json | 18 ++++++++++ backend/tests/drafts_move_taken.rs | 2 +- backend/tests/drafts_save_follows_move.rs | 33 +++++++++++++++++++ backend/windmill-api/src/drafts.rs | 3 +- backend/windmill-common/src/user_drafts.rs | 27 ++++++++++++++- .../sessions/sessionRuntime.svelte.ts | 29 ++++++++-------- .../flows/edit/[...path]/+page.svelte | 7 ++++ .../scripts/edit/[...path]/+page.svelte | 12 ++++--- 8 files changed, 107 insertions(+), 24 deletions(-) create mode 100644 backend/.sqlx/query-dac419c05715b2cdd965dbca1221ccdb37e078a7473e554f15b9c1f2ef6cdb95.json diff --git a/backend/.sqlx/query-dac419c05715b2cdd965dbca1221ccdb37e078a7473e554f15b9c1f2ef6cdb95.json b/backend/.sqlx/query-dac419c05715b2cdd965dbca1221ccdb37e078a7473e554f15b9c1f2ef6cdb95.json new file mode 100644 index 0000000000..6b641daf93 --- /dev/null +++ b/backend/.sqlx/query-dac419c05715b2cdd965dbca1221ccdb37e078a7473e554f15b9c1f2ef6cdb95.json @@ -0,0 +1,18 @@ +{ + "db_name": "PostgreSQL", + "query": "INSERT INTO draft_move (workspace_id, typ, old_path, new_path, email)\n SELECT m.workspace_id, m.typ, m.old_path, $4, $5::text\n FROM draft_move m\n WHERE m.workspace_id = $1 AND m.typ::text = ANY($2::text[])\n AND m.new_path = $3 AND m.email IS DISTINCT FROM $5::text\n AND NOT EXISTS (\n SELECT 1 FROM draft_move o\n WHERE o.workspace_id = m.workspace_id AND o.typ = m.typ\n AND o.old_path = m.old_path AND o.email = $5::text\n )", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "TextArray", + "Text", + "Varchar", + "Text" + ] + }, + "nullable": [] + }, + "hash": "dac419c05715b2cdd965dbca1221ccdb37e078a7473e554f15b9c1f2ef6cdb95" +} diff --git a/backend/tests/drafts_move_taken.rs b/backend/tests/drafts_move_taken.rs index 876fcaf577..eee01c79dd 100644 --- a/backend/tests/drafts_move_taken.rs +++ b/backend/tests/drafts_move_taken.rs @@ -95,7 +95,7 @@ async fn test_draft_move_refuses_the_other_app_kind(db: Pool) -> anyho let body = resp.text().await?; assert_eq!(status, 400, "move onto a classic app draft was allowed: {body}"); assert!( - body.contains("already have a app draft"), + body.contains("already have a draft at 'u/test-user/mvtaken_app' (app)"), "the refusal did not name the occupying kind: {body}" ); diff --git a/backend/tests/drafts_save_follows_move.rs b/backend/tests/drafts_save_follows_move.rs index 7cb8b725a3..3597431825 100644 --- a/backend/tests/drafts_save_follows_move.rs +++ b/backend/tests/drafts_save_follows_move.rs @@ -269,3 +269,36 @@ async fn test_a_teammates_draft_follows_with_its_base(db: Pool) -> any ); Ok(()) } + +/// An item move and then the owner's own move of what is left: the two records have +/// different scopes, so the owner's move has to extend the chain in its own scope or +/// a save addressed to the first path stops at the abandoned middle one. +#[sqlx::test(fixtures("base", "drafts_save_follows_move"))] +async fn test_an_owner_move_extends_an_item_move(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + rename(port, HEAD_HASH, "u/test-user/follow_b").await?; + // Archiving the script at the new path leaves the carried draft as a draft-only + // item, which its owner can move through `/drafts/move`. + sqlx::query("UPDATE script SET archived = true WHERE path = 'u/test-user/follow_b'") + .execute(&db) + .await?; + let resp = reqwest::Client::new() + .post(format!( + "http://localhost:{port}/api/w/test-workspace/drafts/move/script/u/test-user/follow_b" + )) + .header("Authorization", "Bearer SECRET_TOKEN") + .json(&json!({ "new_path": "u/test-user/follow_c" })) + .send() + .await?; + assert!(resp.status().is_success(), "move failed: {}", resp.text().await?); + + assert_eq!( + save_at(port, "u/test-user/follow_a", "after both moves").await?, + "u/test-user/follow_c", + "a save at the first path stopped at the path the owner's move left" + ); + Ok(()) +} diff --git a/backend/windmill-api/src/drafts.rs b/backend/windmill-api/src/drafts.rs index ccee9ead3a..d2bf1532a5 100644 --- a/backend/windmill-api/src/drafts.rs +++ b/backend/windmill-api/src/drafts.rs @@ -783,7 +783,8 @@ async fn move_draft( } else if let Some(occupant) = row.at_target.filter(|_| new_path != path) { // Naming the kind matters for the app pair: a classic-app draft refusing a // raw-app move is invisible in the raw-app list the caller is looking at. - format!("You already have a {} draft at '{new_path}'", occupant.replace('_', " ")) + let occupant = occupant.replace('_', " "); + format!("You already have a draft at '{new_path}' ({occupant})") } else { format!("You have no draft at '{path}'") })); diff --git a/backend/windmill-common/src/user_drafts.rs b/backend/windmill-common/src/user_drafts.rs index 6b9f74e7c4..ebde949871 100644 --- a/backend/windmill-common/src/user_drafts.rs +++ b/backend/windmill-common/src/user_drafts.rs @@ -720,7 +720,8 @@ pub async fn move_drafts_for_path( /// addressed to `old_path` lands on them (see `update_draft`). `email` scopes the /// record to one user's draft-only move; `None` is a deployed item's move, for everyone. /// -/// Kept to one hop: records pointing at `old_path` are re-pointed, and records +/// Kept to one hop: records pointing at `old_path` are re-pointed (an owner's move +/// copies another scope's into its own rather than re-pointing it), 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 @@ -765,6 +766,30 @@ pub async fn record_draft_move( ) .execute(&mut **tx) .await?; + // An owner's move must not re-point what everyone else follows, so the records + // ending at `old_path` in another scope are copied into this one: a save addressed + // to the start of that chain still reaches this destination in one hop. + if email.is_some() { + sqlx::query!( + "INSERT INTO draft_move (workspace_id, typ, old_path, new_path, email) + SELECT m.workspace_id, m.typ, m.old_path, $4, $5::text + FROM draft_move m + WHERE m.workspace_id = $1 AND m.typ::text = ANY($2::text[]) + AND m.new_path = $3 AND m.email IS DISTINCT FROM $5::text + AND NOT EXISTS ( + SELECT 1 FROM draft_move o + WHERE o.workspace_id = m.workspace_id AND o.typ = m.typ + AND o.old_path = m.old_path AND o.email = $5::text + )", + w_id, + &typs as &[&str], + old_path, + new_path, + email, + ) + .execute(&mut **tx) + .await?; + } sqlx::query!( "INSERT INTO draft_move (workspace_id, typ, old_path, new_path, email) SELECT $1, t::draft_kind, $3, $4, $5 FROM unnest($2::text[]) t", diff --git a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts index 4aab4cff08..ef71e6d86e 100644 --- a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts @@ -607,9 +607,6 @@ function createRuntime(session: Session): SessionRuntime { saved.val = undefined } await initFlow(aiDraft, store, stateStore, workspace) - // 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 @@ -618,7 +615,8 @@ function createRuntime(session: Session): SessionRuntime { // No local draft yet — seed from `result.draft ?? result`. const result = await FlowService.getFlowByPath({ workspace, path, getDraft: true }) saved.val = result as SavedFlow - const flow: Flow = ((result as SavedFlow).draft ?? (result as Flow)) as Flow + const serverDraft = (result as SavedFlow).draft as Flow | undefined + const flow: Flow = (serverDraft ?? (result as Flow)) as Flow // 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 — @@ -631,7 +629,10 @@ 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 == null) + // A draft keeps the base it forked from, unknown included (it then falls + // back to the timestamps); only a fresh checkout takes the head, which is + // also what keeps it from always diffing. See loadScript. + if (deployedVersionId != null && store.val && !serverDraft) store.val.version_id = deployedVersionId slot.loadedPath = path slot.loadedWorkspace = workspace @@ -697,12 +698,6 @@ function createRuntime(session: Session): SessionRuntime { schema: emptySchema(), language: (aiDraft.language ?? 'bun') as any } - // 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 if (aiDraft.language) baseline.language = aiDraft.language if (aiDraft.summary !== undefined) baseline.summary = aiDraft.summary @@ -717,11 +712,13 @@ function createRuntime(session: Session): SessionRuntime { saved.val = result as SavedScript // Clone before mutating, else `baseline` aliases `result` and // `baseline.parent_hash` corrupts the diff baseline. - const baseline = structuredClone( - ((result as SavedScript).draft as NewScript | undefined) ?? (result as NewScript) - ) - // 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 + const serverDraft = (result as SavedScript).draft as NewScript | undefined + const baseline = structuredClone(serverDraft ?? (result as NewScript)) + // Only a fresh checkout forks from the head. A draft keeps the base it has, + // unknown included: `draft.base` is derived from `parent_hash` on every + // save, so stamping the head over it would say this draft is up to date + // when it is not. An unknown base falls back to the timestamps. + if (!serverDraft) 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 — diff --git a/frontend/src/routes/(root)/(logged)/flows/edit/[...path]/+page.svelte b/frontend/src/routes/(root)/(logged)/flows/edit/[...path]/+page.svelte index 699749395a..97b636e4b0 100644 --- a/frontend/src/routes/(root)/(logged)/flows/edit/[...path]/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/flows/edit/[...path]/+page.svelte @@ -367,6 +367,13 @@ const effectiveFlow: Flow = draftFromBackend ? ({ ...deployedFlow, ...draftFromBackend } as Flow) : (deployedFlow as Flow) + // The merge above would hand a draft with no base the deployed one, and the next + // autosave would persist it as `draft.base`: a draft behind the deploy would read + // as up to date after one open. A draft keeps the base it has, unknown included + // (staleness then falls back to the timestamps). + if (draftFromBackend && (draftFromBackend as any).version_id == null) { + delete (effectiveFlow as any).version_id + } savedFlow = structuredClone($state.snapshot(effectiveFlow)) as Flow // Baseline for the autosave `discardIf`: the deployed flow WITHOUT the // draft overlay (matches the unedited seed when no draft exists). diff --git a/frontend/src/routes/(root)/(logged)/scripts/edit/[...path]/+page.svelte b/frontend/src/routes/(root)/(logged)/scripts/edit/[...path]/+page.svelte index 6ae4771d1a..f0f9546160 100644 --- a/frontend/src/routes/(root)/(logged)/scripts/edit/[...path]/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/scripts/edit/[...path]/+page.svelte @@ -355,11 +355,13 @@ ? { ...deployedScript, ...draftFromBackend } : (deployedScript as EditableScript) savedScript = structuredClone($state.snapshot(effectiveScript)) - // The draft's base is the version it forked from and only the user moves - // it (by discarding or rebasing). Seeding it from the head here would let - // the next autosave persist the head as the base, so a draft behind the - // deploy reads as up to date after one open. - const parentHash = topHash ?? backendScript.draft_base ?? backendScript.hash + // The draft's base is the version it forked from and only the user moves it + // (by discarding or rebasing). A draft keeps the base it has, unknown included + // (staleness then falls back to the timestamps); seeding the head over it here + // would let the next autosave persist the head as the base, so a draft behind + // the deploy would read as up to date after one open. Only a fresh checkout + // forks from the head. + const parentHash = topHash ?? (hasOwnDraft ? backendScript.draft_base : backendScript.hash) // Baseline for the autosave `discardIf`: the deployed script with the // same `parent_hash` graft the seed below applies and the schema as the // mounted editor holds it, so the unedited draft compares equal.