mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-14 08:02:58 +00:00
fix(editors): confirm a draft is gone from what landed, and follow a rename that committed
The discard verdict read the sync pipeline's own state, which settles whichever write it ends up sending: the form stays editable after Discard, so typing then replaces the queued delete with an upsert and the pipeline reports success for it. The draft-state hint is written by the POST that actually landed, so it answers what the pipeline cannot — whether the draft is still there. A multi-workspace save writes in sequence and a later workspace throwing aborts the rest, but what the acting workspace wrote is deployed either way. Both saves now report that path, so a host is never left pointing at one the item has already moved off. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KcyBRxE8j3ujCYkum9KrC4
This commit is contained in:
co-authored by
Claude Opus 5
parent
1381536ec6
commit
83cd40a6dd
@@ -354,6 +354,11 @@
|
||||
// the form at a linked workspace, and a rename made there is that workspace's
|
||||
// alone — reporting it would move a host that is looking at this one.
|
||||
const savedPath = payloads.find((pl) => pl.ws === effectiveWorkspace)?.s.path ?? from
|
||||
// Set once the acting workspace's own write has committed. The workspaces are
|
||||
// written in sequence and a later one throwing aborts the rest, but what this
|
||||
// one wrote is already deployed — a caller told nothing would stay pointed at
|
||||
// a path it has moved off.
|
||||
let committed: string | undefined = undefined
|
||||
try {
|
||||
for (const { ws, s, ini, existed } of payloads) {
|
||||
if (existed) {
|
||||
@@ -389,6 +394,7 @@
|
||||
// `remove`. See VariableEditor for the full rationale.
|
||||
initialStates[ws] = s
|
||||
existedInitially[ws] = true
|
||||
if (ws === effectiveWorkspace) committed = s.path
|
||||
// `s.path`, not the reported one: each workspace settles against the path
|
||||
// its own write used.
|
||||
settleDraftAfterWrite('resource', s, states[ws]?.draft, from, s.path, { workspace: ws })
|
||||
@@ -405,7 +411,7 @@
|
||||
return savedPath
|
||||
} catch (err) {
|
||||
sendUserToast(`Could not save resource: ${err.body ?? err.message}`, true)
|
||||
return undefined
|
||||
return committed
|
||||
}
|
||||
}
|
||||
</script>
|
||||
|
||||
@@ -299,6 +299,21 @@
|
||||
// the form at a linked workspace, and a rename made there is that workspace's
|
||||
// alone — reporting it would move a host that is looking at this one.
|
||||
const savedPath = payloads.find((pl) => pl.ws === fromWs)?.s.path ?? from
|
||||
// Set once the acting workspace's own write has committed. The workspaces are
|
||||
// written in sequence and a later one throwing aborts the rest, but what this
|
||||
// one wrote is already deployed — a host told nothing would stay pointed at a
|
||||
// path it has moved off.
|
||||
let committed: string | undefined = undefined
|
||||
// Follow the rename locally and tell the host, for whatever committed. Guarded
|
||||
// on this editor still being the one that was saved: re-pointed, `editPath` is
|
||||
// the variable it moved to.
|
||||
const reportSaved = (saved: string | undefined) => {
|
||||
if (editPath === from) {
|
||||
if (saved && saved !== editPath) editPath = saved
|
||||
drawer?.closeDrawer()
|
||||
}
|
||||
onSaved?.(saved, from, fromWs)
|
||||
}
|
||||
try {
|
||||
for (const { ws, s, ini, existed } of payloads) {
|
||||
if (existed) {
|
||||
@@ -342,6 +357,7 @@
|
||||
// resets the handle to it via `discard` (not `remove` — blanking the cell
|
||||
// to `undefined` reads as dirty), and keeps an edit made mid-request. Each
|
||||
// workspace settles against the path its own write used, not the reported one.
|
||||
if (ws === fromWs) committed = s.path
|
||||
settleDraftAfterWrite('variable', s, states[ws]?.draft, from ?? '', s.path, {
|
||||
workspace: ws
|
||||
})
|
||||
@@ -353,17 +369,10 @@
|
||||
edit ? `Updated variable in ${payloads.length} workspace(s)` : `Created variable`
|
||||
)
|
||||
dispatch('create')
|
||||
// Only while this editor is still the one that was saved: re-pointed, the
|
||||
// path below is the variable it moved to.
|
||||
if (editPath === from) {
|
||||
// A rename moved the item; the drawer host closes over it, but an inline one
|
||||
// stays mounted, so follow the new path here and tell the host about it.
|
||||
if (savedPath && savedPath !== editPath) editPath = savedPath
|
||||
drawer?.closeDrawer()
|
||||
}
|
||||
onSaved?.(savedPath, from, fromWs)
|
||||
reportSaved(savedPath)
|
||||
} catch (err) {
|
||||
sendUserToast(`Could not save variable: ${err.body}`, true)
|
||||
if (committed) reportSaved(committed)
|
||||
}
|
||||
}
|
||||
</script>
|
||||
|
||||
@@ -4,6 +4,7 @@ import { deepEqual } from 'fast-equals'
|
||||
import { workspaceStore } from './stores'
|
||||
import { readFieldsRecursively } from './utils'
|
||||
import { UserDraftDbSyncer } from './userDraftDbSyncer.svelte'
|
||||
import { getLocalDraftHint } from './localDraftHints.svelte'
|
||||
import type { UserDraftItemKind } from './gen'
|
||||
|
||||
export type { UserDraftItemKind }
|
||||
@@ -291,22 +292,21 @@ export function settleDraftAfterWrite<V>(
|
||||
}
|
||||
|
||||
/**
|
||||
* Wait for a queued draft delete to actually land, reporting whether it did. The
|
||||
* delete rides the autosave debouncer, so a caller that acts on it — a host that
|
||||
* leaves an editor whose item the discard removed — would otherwise navigate onto
|
||||
* a row that is still there, or away from a delete that failed.
|
||||
* Wait for a queued draft delete to actually land, reporting whether the draft is
|
||||
* gone. A caller acts on this — a host leaves an editor whose item the discard
|
||||
* removed — so it has to be the outcome, not the pipeline's state: the delete
|
||||
* shares its queue with the edits, and the form stays editable after Discard, so
|
||||
* it can be displaced by a newer upsert as easily as it can fail. The hint is
|
||||
* written by whichever POST landed, which is what tells the two apart.
|
||||
*/
|
||||
export async function flushDraftDelete(
|
||||
itemKind: UserDraftItemKind,
|
||||
path: string,
|
||||
opts?: UserDraftOptions
|
||||
): Promise<boolean> {
|
||||
const query = { workspace: resolveWorkspace(opts), itemKind, path }
|
||||
await UserDraftDbSyncer.flush(query)
|
||||
return (
|
||||
UserDraftDbSyncer.getState(query).state !== 'failed' &&
|
||||
UserDraftDbSyncer.getConflict(query).conflict === undefined
|
||||
)
|
||||
const workspace = resolveWorkspace(opts)
|
||||
await UserDraftDbSyncer.flush({ workspace, itemKind, path })
|
||||
return getLocalDraftHint(workspace, itemKind, path) === false
|
||||
}
|
||||
|
||||
export type UserDraftHandle<V> = {
|
||||
|
||||
@@ -0,0 +1,48 @@
|
||||
import { describe, it, expect, beforeEach, vi } from 'vitest'
|
||||
|
||||
// The flush stands in for the POST it drives: whatever write actually lands is
|
||||
// what publishes the hint, which is the only thing that tells a delete apart
|
||||
// from an edit that displaced it.
|
||||
let landed: 'delete' | 'upsert' | 'nothing' = 'delete'
|
||||
vi.mock('./userDraftDbSyncer.svelte', () => ({
|
||||
UserDraftDbSyncer: {
|
||||
flush: vi.fn(async ({ workspace, itemKind, path }: any) => {
|
||||
if (landed === 'nothing') return
|
||||
const { setLocalDraftHint } = await import('./localDraftHints.svelte')
|
||||
setLocalDraftHint(workspace, itemKind, path, landed === 'upsert')
|
||||
}),
|
||||
save: vi.fn()
|
||||
}
|
||||
}))
|
||||
vi.mock('./gen', () => ({ DraftService: { updateDraft: vi.fn() } }))
|
||||
vi.mock('./gen/core/OpenAPI', () => ({ OpenAPI: { BASE: '' } }))
|
||||
|
||||
import { flushDraftDelete } from './userDraft.svelte'
|
||||
|
||||
let n = 0
|
||||
let path = ''
|
||||
beforeEach(() => {
|
||||
// A fresh key per case: hints persist by design, so a reused one would carry
|
||||
// the previous case's answer.
|
||||
path = `u/me/v${n++}`
|
||||
})
|
||||
|
||||
describe('flushDraftDelete', () => {
|
||||
it('confirms a delete that landed', async () => {
|
||||
landed = 'delete'
|
||||
expect(await flushDraftDelete('variable', path, { workspace: 'ws' })).toBe(true)
|
||||
})
|
||||
|
||||
// Discard leaves the form editable, so typing after it can replace the queued
|
||||
// `value: null` with an upsert. The pipeline settles either way; only the item
|
||||
// still being there tells the caller not to leave the editor.
|
||||
it('rejects a delete displaced by a later edit', async () => {
|
||||
landed = 'upsert'
|
||||
expect(await flushDraftDelete('variable', path, { workspace: 'ws' })).toBe(false)
|
||||
})
|
||||
|
||||
it('rejects a delete that never landed', async () => {
|
||||
landed = 'nothing'
|
||||
expect(await flushDraftDelete('variable', path, { workspace: 'ws' })).toBe(false)
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user