starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-09-21 00:02:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: clear rows at move targets, save exact diffs, keep toggles clean

Browse Source 
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ViTUkt4czrvZdxdWwxqRAQ
This commit is contained in:
Diego Imbert
2026-09-11 18:04:47 +02:00
co-authored by Claude Opus 5
parent 92a6ab6c4e
commit 82c2b057d1
2 changed files with 74 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files
frontend/src/lib/itemStore.svelte.ts
+36 -21
View File
@@ -175,8 +175,9 @@ class Entry<V> {
stopWatch: (() => void) | undefined
/** The value as last observed, serialized: `touch` acts only on a real change. */
private seen: string | undefined
/** The row last handed to the syncer (`null`: none). What `reconcile` diffs against. */
private row: string | null = null
/** The row last handed to the syncer (`null`: none, `undefined`: not known — the next
* reconcile writes whatever the rule says). What `reconcile` diffs against. */
private row: string | null | undefined = null
/** Counts value changes, so a load can tell whether one landed while it was in flight. */
private changes = 0
private queue: Promise<unknown> = Promise.resolve()
@@ -338,7 +339,9 @@ class Entry<V> {
const from = this.key
// A create or a draft-only item is always a write; anything else already deployed is
// saved — which is also what makes a second click behind an in-flight save a no-op.
if (this.origin === 'deployed' && draftValuesEqual(sent, this.deployed)) {
// Compared exactly, not as drafts are: a field the draft comparison ignores (a
// schedule's run-as) is still one an explicit save must send.
if (this.origin === 'deployed' && serialize(sent) === serialize(this.deployed)) {
return { ok: true, path: from.path, moved: false }
}
const to = (adapter.pathOf ?? ((v: V) => (v as { path: string }).path))(sent)
@@ -397,52 +400,62 @@ class Entry<V> {
await Promise.all(keys.filter((k) => !isTemporaryPath(k.path)).map((k) => this.ports.flush(k)))
}
/** Re-key to `path`. The row at the path left behind follows from nothing living there. */
/**
* Re-key to `path`. The row at the path left behind follows from nothing living there. The
* one at `path` was never read: whatever it holds predates the write that just landed there,
* so it is reconciled like any other.
*/
private moveTo(path: string): void {
const from = this.key
if (!isTemporaryPath(from.path)) {
this.ports.hint(from, false)
if (this.row !== null) this.ports.write(from, null)
}
this.row = null
this.row = undefined
this.key = { ...from, path }
this.store.rekey(this, from)
}
/**
* Run `write`, then fold `fields` into the deployed side: the server holds them now. The
* value takes them at once; on failure it gives back those it still holds.
* Write `fields` to the server through `write`. Both sides take them at once — the value
* because the user asked, the baseline because the server is about to hold them — so the
* request never reads as a draft. On failure each side gives back those it still holds.
*/
patch(fields: Partial<V>, write: (key: ItemKey) => Promise<unknown>): Promise<CommandOutcome> {
this.pristine = false
const baseKey = this.origin === 'new' ? 'template' : 'deployed'
const before = snapshot(this[baseKey]) as Record<string, unknown> | undefined
const sent = snapshot(fields) as Record<string, unknown>
if (before !== undefined) this[baseKey] = { ...before, ...sent } as V
if (this.value !== undefined) Object.assign(this.value as object, fields)
this.touch()
return this.run(async () => {
try {
await write(this.key)
} catch (e) {
const base = (this.origin === 'new' ? this.template : this.deployed) as
| Record<string, unknown>
| undefined
if (this.value !== undefined && base !== undefined) {
const reverted = snapshot(this.value) as Record<string, unknown>
const giveBack = (side: V | undefined): V | undefined => {
if (side === undefined || before === undefined) return undefined
const out = snapshot(side) as Record<string, unknown>
let changed = false
for (const [k, v] of Object.entries(fields)) {
if (deepEqual(reverted[k], v)) {
reverted[k] = base[k]
for (const [k, v] of Object.entries(sent)) {
if (deepEqual(out[k], v)) {
out[k] = before[k]
changed = true
}
}
if (changed) this.replaceValue(reverted as V)
return changed ? (out as V) : undefined
}
const base = giveBack(this[baseKey])
if (base !== undefined) this[baseKey] = base
const value = giveBack(this.value)
if (value !== undefined) this.replaceValue(value)
this.error = errorMessage(e)
this.reconcile()
return { ok: false, error: this.error }
}
this.error = undefined
if (this.origin === 'deployed' && this.deployed !== undefined) {
this.deployed = { ...this.deployed, ...snapshot(fields) }
}
// A save that landed while this waited set the baseline to what it sent.
if (this[baseKey] !== undefined) this[baseKey] = { ...this[baseKey], ...sent } as V
this.touch()
this.reconcile()
return { ok: true }
@@ -696,8 +709,10 @@ export function createItemStore(ports: ItemRowPort) {
*/
function saveEach<V>(items: ItemHandle<V>[]): Promise<SaveOutcome[]> {
const pending = items.map((item) => {
if (!(item instanceof Handle)) throw new Error('saveEach takes the handles useItems returns')
const { entry, adapter } = item as Handle<V>
// `useItem` hands out a forwarder; the item it forwards to is its `current`.
const handle = item instanceof Handle ? item : (item as { current?: unknown }).current
if (!(handle instanceof Handle)) throw new Error('saveEach takes handles from useItem(s)')
const { entry, adapter } = handle as Handle<V>
return { entry, adapter, started: entry.begin(), asked: entry.ask() }
})
return (async () => {
frontend/src/lib/itemStore.test.ts
+38 -10
View File
@@ -240,12 +240,37 @@ describe('item store: commands', () => {
{ load: async () => ({ deployed: { path: 's', enabled: true } }) } as ItemAdapter<Sched>
)
await settle()
const outcome = await item.patch({ enabled: false }, async () => {
throw new Error('refused')
})
expect(outcome).toEqual({ ok: false, error: 'refused' })
expect(item.value).toEqual({ path: 's', enabled: true })
const refusal = deferred()
const toggling = item.patch({ enabled: false }, () => refusal.promise)
// In flight the toggle is the server's to hold, not a draft of the user's.
expect(item.dirty).toBe(false)
expect(rows.writes).toEqual([])
refusal.reject(new Error('refused'))
expect(await toggling).toEqual({ ok: false, error: 'refused' })
expect(item.value).toEqual({ path: 's', enabled: true })
expect(item.deployed).toEqual({ path: 's', enabled: true })
expect(rows.writes).toEqual([])
})
it('saves a change the draft comparison ignores', async () => {
type Sched = { path: string; permissioned_as?: string }
const rows = fakeRows()
const store = createItemStore(rows.port)
const writes: unknown[] = []
const { handle: item } = store.acquire(
{ workspace: 'w', kind: 'trigger_schedule', path: 's' },
{ workspace: 'w', path: 's' },
{
load: async () => ({ deployed: { path: 's', permissioned_as: 'u/a' } }),
write: async (ctx) => void writes.push(ctx.value)
} as ItemAdapter<Sched>
)
await settle()
item.value = { path: 's', permissioned_as: 'u/b' }
expect(item.dirty).toBe(false)
expect(await item.save()).toMatchObject({ ok: true })
expect(writes).toEqual([{ path: 's', permissioned_as: 'u/b' }])
})
})
@@ -323,7 +348,7 @@ describe('item store: origins', () => {
expect(rows.writes).toEqual([{ path: 'u/me/r', value: null }])
})
it('creates under a temporary path, then moves to the real one with no row at either', async () => {
it('creates under a temporary path, then moves to the real one and clears any row there', async () => {
const rows = fakeRows()
const temp = newItemPath()
const a = adapter({})
@@ -340,12 +365,13 @@ describe('item store: origins', () => {
expect(item.key.path).toBe('u/me/created')
expect(item.origin).toBe('deployed')
expect(item.dirty).toBe(false)
expect(rows.writes).toEqual([])
// Nothing under the temporary path; a draft left at the real one predates the create.
expect(rows.writes).toEqual([{ path: 'u/me/created', value: null }])
expect(store.bridge.read('w', 'resource', 'u/me/created')).toEqual({ value: undefined })
expect(store.bridge.read('w', 'resource', temp)).toBeUndefined()
})
it('moves a renamed item and deletes the row it left behind', async () => {
it('moves a renamed item and clears the rows at both paths', async () => {
const rows = fakeRows()
const { item } = await open(rows, adapter({ deployed: deployedRes }))
item.value = { ...deployedRes, path: 'u/me/renamed' }
@@ -354,8 +380,10 @@ describe('item store: origins', () => {
])
expect(await item.save()).toEqual({ ok: true, path: 'u/me/renamed', moved: true })
expect(rows.writes.at(-1)).toEqual({ path: 'u/me/r', value: null })
expect(rows.writes.some((w) => w.path === 'u/me/renamed')).toBe(false)
expect(rows.writes.slice(1)).toEqual([
{ path: 'u/me/r', value: null },
{ path: 'u/me/renamed', value: null }
])
})
})