From b44f031d35b5090eedf2530fc546b1b924ec56c8 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Thu, 28 May 2026 16:02:02 +0200 Subject: [PATCH] feat: support draft deletion via sync (value: null) with same conflict semantics --- ...420468770630c1805058edd14679062162b8c.json | 17 +++ ...cd6885cc499823ee1d52bf2631dd3413f310.json} | 4 +- backend/windmill-api/openapi.yaml | 21 +++- backend/windmill-api/src/drafts.rs | 105 ++++++++++++------ .../UserDraftConflictModal.svelte | 37 ++++-- frontend/src/lib/userDraft.svelte.ts | 58 ++++++++-- frontend/src/lib/userDraftDbSyncer.svelte.ts | 10 +- 7 files changed, 192 insertions(+), 60 deletions(-) create mode 100644 backend/.sqlx/query-0f7ad07f2bef7e55d6fc15db11d420468770630c1805058edd14679062162b8c.json rename backend/.sqlx/{query-ddd58ed070d8c0bbbb060cd881ff618561958f183815f3dc339cf06c6f40946a.json => query-29190ba48f4986eaf35247e9754bcd6885cc499823ee1d52bf2631dd3413f310.json} (52%) diff --git a/backend/.sqlx/query-0f7ad07f2bef7e55d6fc15db11d420468770630c1805058edd14679062162b8c.json b/backend/.sqlx/query-0f7ad07f2bef7e55d6fc15db11d420468770630c1805058edd14679062162b8c.json new file mode 100644 index 0000000000..e27ca74d56 --- /dev/null +++ b/backend/.sqlx/query-0f7ad07f2bef7e55d6fc15db11d420468770630c1805058edd14679062162b8c.json @@ -0,0 +1,17 @@ +{ + "db_name": "PostgreSQL", + "query": "DELETE FROM draft\n WHERE workspace_id = $1\n AND email = $2\n AND path = $3\n AND typ = $4", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "0f7ad07f2bef7e55d6fc15db11d420468770630c1805058edd14679062162b8c" +} diff --git a/backend/.sqlx/query-ddd58ed070d8c0bbbb060cd881ff618561958f183815f3dc339cf06c6f40946a.json b/backend/.sqlx/query-29190ba48f4986eaf35247e9754bcd6885cc499823ee1d52bf2631dd3413f310.json similarity index 52% rename from backend/.sqlx/query-ddd58ed070d8c0bbbb060cd881ff618561958f183815f3dc339cf06c6f40946a.json rename to backend/.sqlx/query-29190ba48f4986eaf35247e9754bcd6885cc499823ee1d52bf2631dd3413f310.json index 00a5762e99..ce13fd5351 100644 --- a/backend/.sqlx/query-ddd58ed070d8c0bbbb060cd881ff618561958f183815f3dc339cf06c6f40946a.json +++ b/backend/.sqlx/query-29190ba48f4986eaf35247e9754bcd6885cc499823ee1d52bf2631dd3413f310.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "INSERT INTO draft (workspace_id, email, path, typ, value, created_at)\n VALUES ($1, $2, $3, $4, $5::text::json, now())\n ON CONFLICT (workspace_id, path, typ, email) WHERE email IS NOT NULL\n DO UPDATE SET value = EXCLUDED.value, created_at = now()\n RETURNING created_at", + "query": "INSERT INTO draft (workspace_id, email, path, typ, value, created_at)\n VALUES ($1, $2, $3, $4, $5::text::json, now())\n ON CONFLICT (workspace_id, path, typ, email) WHERE email IS NOT NULL\n DO UPDATE SET value = EXCLUDED.value, created_at = now()\n RETURNING created_at", "describe": { "columns": [ { @@ -22,5 +22,5 @@ false ] }, - "hash": "ddd58ed070d8c0bbbb060cd881ff618561958f183815f3dc339cf06c6f40946a" + "hash": "29190ba48f4986eaf35247e9754bcd6885cc499823ee1d52bf2631dd3413f310" } diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 690bce9237..5b442d9748 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -7831,11 +7831,13 @@ paths: typ: type: string description: UserDraft item kind. Stored verbatim; the client owns the value set. - value: {} + value: + nullable: true + description: Draft content. `null` (or omitted) means delete the draft at this path. Conflict semantics apply the same way to deletes. force: type: boolean description: Skip the conflict check for this entry and overwrite the server copy. - required: [path, typ, value] + required: [path, typ] required: [drafts] responses: "200": @@ -7876,6 +7878,16 @@ paths: type: string format: date-time required: [status, path, typ, created_at] + - type: object + properties: + status: + type: string + enum: [deleted] + path: + type: string + typ: + type: string + required: [status, path, typ] - type: object properties: status: @@ -7889,14 +7901,15 @@ paths: server_created_at: type: string format: date-time - incoming_value: {} + incoming_value: + nullable: true + description: The value the client tried to push, or `null` if the client attempted a delete. required: - status - path - typ - server_value - server_created_at - - incoming_value current_timestamp: type: string format: date-time diff --git a/backend/windmill-api/src/drafts.rs b/backend/windmill-api/src/drafts.rs index 661ea8ce41..12b90770ee 100644 --- a/backend/windmill-api/src/drafts.rs +++ b/backend/windmill-api/src/drafts.rs @@ -34,7 +34,10 @@ pub struct IncomingDraft { /// `trigger_*`, ...). Stored verbatim — no server-side validation, /// since the set of kinds is owned by the client. pub typ: String, - pub value: sqlx::types::Json>, + /// `null` (or omitted) means delete the draft at this path. Conflict + /// semantics apply the same way to deletions as to upserts. + #[serde(default)] + pub value: Option>>, /// When true, skip the conflict check for this entry and overwrite the /// server copy. Only the matching entry is forced — other entries in /// the same batch still run through the normal conflict check. @@ -68,15 +71,20 @@ pub enum DraftSyncStatus { typ: String, created_at: chrono::DateTime, }, + Deleted { + path: String, + typ: String, + }, Rejected { path: String, typ: String, /// Current server copy at conflict-detection time. server_value: sqlx::types::Json>, server_created_at: chrono::DateTime, - /// The value the client tried to push. Echoed back so the client can - /// retry with `force = true` without re-reading its local state. - incoming_value: sqlx::types::Json>, + /// The value the client tried to push. `None` when the client + /// attempted a delete; the modal interprets this as "you tried to + /// delete, but the server has a newer version". + incoming_value: Option>>, }, } @@ -94,10 +102,6 @@ async fn sync_drafts( Json(req): Json, ) -> Result> { let email = &authed.email; - let current_timestamp = sqlx::query_scalar!("SELECT now()") - .fetch_one(&db) - .await? - .expect("now() is never null"); let missed_drafts = if let Some(last_sync) = req.last_sync { sqlx::query_as!( @@ -156,40 +160,77 @@ async fn sync_drafts( typ: incoming.typ.clone(), server_value: row.value, server_created_at: row.created_at, - incoming_value: sqlx::types::Json( - serde_json::value::RawValue::from_string( - incoming.value.0.get().to_string(), + incoming_value: incoming.value.as_ref().map(|v| { + sqlx::types::Json( + serde_json::value::RawValue::from_string(v.0.get().to_string()) + .expect("RawValue round-trip"), ) - .expect("RawValue round-trip"), - ), + }), }); continue; } } } - let row = sqlx::query!( - r#"INSERT INTO draft (workspace_id, email, path, typ, value, created_at) - VALUES ($1, $2, $3, $4, $5::text::json, now()) - ON CONFLICT (workspace_id, path, typ, email) WHERE email IS NOT NULL - DO UPDATE SET value = EXCLUDED.value, created_at = now() - RETURNING created_at"#, - &w_id, - email, - incoming.path, - incoming.typ, - serde_json::to_string(&incoming.value).unwrap(), - ) - .fetch_one(&db) - .await?; + match &incoming.value { + Some(value) => { + let row = sqlx::query!( + r#"INSERT INTO draft (workspace_id, email, path, typ, value, created_at) + VALUES ($1, $2, $3, $4, $5::text::json, now()) + ON CONFLICT (workspace_id, path, typ, email) WHERE email IS NOT NULL + DO UPDATE SET value = EXCLUDED.value, created_at = now() + RETURNING created_at"#, + &w_id, + email, + incoming.path, + incoming.typ, + serde_json::to_string(value).unwrap(), + ) + .fetch_one(&db) + .await?; - statuses.push(DraftSyncStatus::Saved { - path: incoming.path.clone(), - typ: incoming.typ.clone(), - created_at: row.created_at, - }); + statuses.push(DraftSyncStatus::Saved { + path: incoming.path.clone(), + typ: incoming.typ.clone(), + created_at: row.created_at, + }); + } + None => { + // Delete-only path. Idempotent: the DELETE is a no-op if + // the row was already gone (concurrent delete from another + // tab) — we still report `Deleted` so the client clears + // its pending state. + sqlx::query!( + r#"DELETE FROM draft + WHERE workspace_id = $1 + AND email = $2 + AND path = $3 + AND typ = $4"#, + &w_id, + email, + incoming.path, + incoming.typ, + ) + .execute(&db) + .await?; + + statuses.push(DraftSyncStatus::Deleted { + path: incoming.path.clone(), + typ: incoming.typ.clone(), + }); + } + } } + // Compute after the inserts so the response's `current_timestamp` is + // >= every just-saved row's `created_at`. Otherwise a client that + // re-syncs immediately would see its own writes as newer than its + // `last_sync` and get rejected on the next push. + let current_timestamp = sqlx::query_scalar!("SELECT now()") + .fetch_one(&db) + .await? + .expect("now() is never null"); + Ok(Json(SyncDraftsResponse { missed_drafts, statuses, diff --git a/frontend/src/lib/components/common/confirmationModal/UserDraftConflictModal.svelte b/frontend/src/lib/components/common/confirmationModal/UserDraftConflictModal.svelte index cc0efa725f..cc7ab521f2 100644 --- a/frontend/src/lib/components/common/confirmationModal/UserDraftConflictModal.svelte +++ b/frontend/src/lib/components/common/confirmationModal/UserDraftConflictModal.svelte @@ -4,6 +4,14 @@ * top of the tree (root layout) so any UserDraft.save call can enqueue * a conflict via `UserDraftConflictStore.enqueue` and have it shown to * the user without coordinating with a route component. + * + * Two conflict shapes: + * • upsert rejected: `incoming_value` is non-null. The user tried to + * save and lost the race. Offer "Overwrite server draft" / "Load + * server draft". + * • delete rejected: `incoming_value` is null. The user tried to + * discard and lost the race. Offer "Delete anyway" / "Load server + * draft". */ import { classNames } from '$lib/utils' import { fade } from 'svelte/transition' @@ -16,12 +24,16 @@ const conflict = $derived(UserDraftConflictStore.current) const open = $derived(conflict !== undefined) + const isDeleteAttempt = $derived( + conflict !== undefined && + (conflict.rejected.incoming_value === null || conflict.rejected.incoming_value === undefined) + ) const lastSyncDate = $derived(UserDraftDbSyncer.getLastSync()) let busy = $state(false) - async function overwriteServer() { + async function forceLocal() { if (!conflict) return busy = true try { @@ -31,14 +43,14 @@ { itemKind: conflict.itemKind, path: conflict.rejected.path, - value: conflict.rejected.incoming_value, + value: isDeleteAttempt ? null : conflict.rejected.incoming_value, force: true } ] }) UserDraftConflictStore.dismiss() } catch (e) { - sendUserToast(`Could not overwrite server draft: ${e.body ?? e.message}`, true) + sendUserToast(`Could not apply local change: ${e.body ?? e.message}`, true) } finally { busy = false } @@ -82,11 +94,20 @@

- Your draft for {conflict.rejected.path} is out of date + {#if isDeleteAttempt} + Couldn't discard your draft for {conflict.rejected.path} + {:else} + Your draft for {conflict.rejected.path} is out of date + {/if}

- Another session saved a newer draft for this {conflict.itemKind} since this tab last - synced. + {#if isDeleteAttempt} + Another session saved a newer draft for this {conflict.itemKind} since this tab last + synced — discarding now would erase changes you haven't seen. + {:else} + Another session saved a newer draft for this {conflict.itemKind} since this tab last + synced. + {/if}

- Overwrite server draft + {isDeleteAttempt ? 'Discard anyway' : 'Overwrite server draft'}
diff --git a/frontend/src/lib/userDraft.svelte.ts b/frontend/src/lib/userDraft.svelte.ts index 3c1efde0c5..8e284256b3 100644 --- a/frontend/src/lib/userDraft.svelte.ts +++ b/frontend/src/lib/userDraft.svelte.ts @@ -111,6 +111,14 @@ type DraftEntry = { itemKind: UserDraftItemKind path: string state: DraftState + /** + * Set to `true` by callers that already know what's on the server + * (e.g. an `onMissedDrafts` echo, or the conflict modal's "Load server + * draft" branch). The acquireEntry `$effect` consumes the flag — + * skipping exactly one pushToSyncer call — to break the otherwise + * inevitable ping-pong between live-cell writes and the syncer. + */ + skipNextSync: boolean /** * Tears down the `$effect.root` scope that owns the entry's * `useLocalStorageValue` reactivity — its `$state` cell and the persist @@ -351,14 +359,14 @@ export function localDraftDiffers( } /** - * Persist a draft via the DbSyncer queue. Internal `_skipSync` opts - * pass-through lets the syncer write missed drafts back to LS without - * re-syncing them. + * Persist a draft via the DbSyncer queue. `value === null` signals a + * delete (matches the wire shape). Internal `_skipSync` opts pass-through + * lets the syncer write missed drafts back to LS without re-syncing. */ function pushToSyncer( itemKind: UserDraftItemKind, path: string, - value: unknown, + value: unknown | null, workspace: string ): void { // Skip sync until a user is signed in — without a session the request @@ -401,6 +409,11 @@ export const UserDraft = { // its initial-write skip armed. const current = untrack(() => entry.state.val as StoredDraft | undefined) const meta = extractMeta(current) + // Arm the sync-skip BEFORE the reactive write, so the cell's + // $effect — which fires on a microtask after `setWithoutPersist` + // — sees the flag and bails. Otherwise a `_skipSync` save would + // still bounce to the server through the live-cell push path. + if (opts?._skipSync) entry.skipNextSync = true entry.state.setWithoutPersist(wrap(value, meta)) persistDirect(localStorageKey(ws, itemKind, path), value, meta) } else { @@ -530,16 +543,19 @@ export const UserDraft = { return readPersisted(localStorageKey(ws, itemKind, path)) !== undefined }, - remove(itemKind: UserDraftItemKind, path: string, opts?: UserDraftOptions): void { + remove(itemKind: UserDraftItemKind, path: string, opts?: SaveOptions): void { const ws = resolveWorkspace(opts) try { localStorage.removeItem(localStorageKey(ws, itemKind, path)) } catch (e) { console.error('UserDraft.remove: localStorage remove failed', e) } + if (!opts?._skipSync) { + pushToSyncer(itemKind, path, null, ws) + } }, - clear(itemKind: UserDraftItemKind, path: string, opts?: UserDraftOptions): void { + clear(itemKind: UserDraftItemKind, path: string, opts?: SaveOptions): void { UserDraft.discard(itemKind, path, undefined, opts) }, @@ -632,7 +648,7 @@ export const UserDraft = { itemKind: UserDraftItemKind, path: string, fallback: V | undefined, - opts?: UserDraftOptions + opts?: SaveOptions ): void { const ws = resolveWorkspace(opts) const mk = mapKey(ws, itemKind, path) @@ -642,6 +658,7 @@ export const UserDraft = { // resetting the in-memory value. Otherwise a timer from the old // entry can outlive unmount and later delete a freshly written // draft for the same key. + if (opts?._skipSync) entry.skipNextSync = true entry.state.setWithoutPersist(wrap(fallback) as StoredDraft | undefined) } try { @@ -649,6 +666,9 @@ export const UserDraft = { } catch (e) { console.error('UserDraft.discard: localStorage remove failed', e) } + if (!opts?._skipSync) { + pushToSyncer(itemKind, path, null, ws) + } }, use( @@ -781,7 +801,9 @@ function acquireEntry( // $effect) to the DbSyncer. Skip the first run — the initial value // came from localStorage or the default, not from a user mutation, so // echoing it back would create a redundant request on every editor - // mount. + // mount. Also honor the entry's `skipNextSync` flag, which callers + // set when they already know what's on the server (missed_drafts + // echo, "Load server draft" in the conflict modal). let firstRun = true $effect(() => { const stored = stateRef!.val @@ -789,13 +811,25 @@ function acquireEntry( firstRun = false return } - const value = stored === undefined ? undefined : (stored.value as unknown) - if (value === undefined) return + const entry = entries.get(mk) + if (entry?.skipNextSync) { + entry.skipNextSync = false + return + } + const value = stored === undefined ? null : (stored.value as unknown) pushToSyncer(itemKind, path, value, workspace) }) }) if (stateRef) { - entries.set(mk, { count: 1, workspace, itemKind, path, state: stateRef, destroyRoot }) + entries.set(mk, { + count: 1, + workspace, + itemKind, + path, + state: stateRef, + skipNextSync: false, + destroyRoot + }) return } // Fallback for the vitest runtime where `$effect.root`'s callback isn't @@ -806,7 +840,7 @@ function acquireEntry( undefined, useLocalStorageOptions ) - entries.set(mk, { count: 1, workspace, itemKind, path, state }) + entries.set(mk, { count: 1, workspace, itemKind, path, state, skipNextSync: false }) } function releaseEntry(mk: string): void { diff --git a/frontend/src/lib/userDraftDbSyncer.svelte.ts b/frontend/src/lib/userDraftDbSyncer.svelte.ts index 25d10d445f..2bbc084612 100644 --- a/frontend/src/lib/userDraftDbSyncer.svelte.ts +++ b/frontend/src/lib/userDraftDbSyncer.svelte.ts @@ -23,11 +23,17 @@ export type RejectedDraft = Extract = { itemKind: UserDraftItemKind path: string - value: V + /** + * Draft content. `null` (or omitted) signals a delete — the server + * removes the row at this path, applying the same conflict semantics + * as an upsert. + */ + value: V | null /** * Skip the conflict check for this single entry and overwrite the * server copy. Used by the conflict-resolution modal's "Overwrite - * server draft" action; routine autosaves leave this `false`. + * server draft" / "Delete anyway" actions; routine autosaves leave + * this `false`. */ force?: boolean }