mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-17 16:03:06 +00:00
fix(drafts): settle nothing for a create, and make a rename's delete final
A create's form cell is the detached handle `useMany` gives an empty path, so the settle addressed its requests to `/drafts/update/<kind>/`: three 404s on every resource, variable and schedule created from a blank form. It has no draft to settle — short-circuit. Following that create's path afterwards re-keyed the blank cell onto the item just made, and the mirror posted it as a draft: every new variable and resource landed with an empty draft over it, secret flag and all. Only a move is followed now. The rename retry loop existed because the form stays editable across the delete's request and each keystroke queued a write behind it. Suspending the old key for the length of the delete removes the race instead of racing it: one attempt is final, and a delete that still will not land is a conflict or a failed request, which no retry changes and which nothing said out loud before. 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
81eb6cf115
commit
724b48eedd
@@ -249,8 +249,10 @@
|
||||
// The editor adopts its own new baseline, so a same-path save needs nothing
|
||||
// here — remounting it would re-read over an edit made during the write.
|
||||
// Only while this is still the resource it saved: re-pointed mid-write,
|
||||
// `path` and the mounted editor are another one's.
|
||||
if (path === from && submitted) path = submitted
|
||||
// `path` and the mounted editor are another one's. And only a move: pointing a
|
||||
// create at the path it just made re-keys the blank form's cell onto it, and
|
||||
// the mirror then posts that blank state as the new item's draft.
|
||||
if (path && path === from && submitted) path = submitted
|
||||
// One report per workspace written, each carrying its own — a linked
|
||||
// workspace's rename moves the tabs acting on it and no others. Reported
|
||||
// even when this drawer has moved on: the write is a fact about the item.
|
||||
|
||||
@@ -375,8 +375,10 @@
|
||||
// `editPath` 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.
|
||||
if (actingPath && actingPath !== editPath) editPath = actingPath
|
||||
// stays mounted, so follow the new path here. Only a move: pointing a create
|
||||
// at the path it just made re-keys the blank form's cell onto it, and the
|
||||
// mirror then posts that blank state as the new item's draft.
|
||||
if (editPath && actingPath && actingPath !== editPath) editPath = actingPath
|
||||
drawer?.closeDrawer()
|
||||
}
|
||||
} catch (err) {
|
||||
|
||||
@@ -2,7 +2,7 @@ import { get } from 'svelte/store'
|
||||
import { onDestroy, untrack } from 'svelte'
|
||||
import { deepEqual } from 'fast-equals'
|
||||
import { workspaceStore } from './stores'
|
||||
import { readFieldsRecursively } from './utils'
|
||||
import { readFieldsRecursively, sendUserToast } from './utils'
|
||||
import { UserDraftDbSyncer } from './userDraftDbSyncer.svelte'
|
||||
import type { UserDraftItemKind } from './gen'
|
||||
|
||||
@@ -317,6 +317,10 @@ export async function settleDraftAfterWrite<V>(
|
||||
savedPath: string,
|
||||
opts?: UserDraftOptions
|
||||
): Promise<void> {
|
||||
// A create has nothing to settle: the blank form's cell is the detached handle
|
||||
// `useMany` hands an empty path, wired to no key, so every request made for one
|
||||
// is addressed to `/drafts/update/<kind>/` and 404s.
|
||||
if (!fromPath) return
|
||||
// A released cell reads as no cell at all, and its draft is then precisely what
|
||||
// nothing is watching any more — except when the release itself is what took
|
||||
// the handle away mid-write, in which case what it held still speaks for the
|
||||
@@ -324,20 +328,30 @@ export async function settleDraftAfterWrite<V>(
|
||||
const held = live !== undefined ? live : takeReleasedValue<V>(itemKind, fromPath, opts)
|
||||
const diverged = held !== undefined && !draftValuesEqual(held, written)
|
||||
if (savedPath === fromPath && diverged) return
|
||||
// Sent before this resolves, not left on the keystroke debounce: callers report
|
||||
// the write and remount on it, and both read a cell this has to have finished
|
||||
// resolving — including through a frame with a draft store of its own.
|
||||
UserDraft.discard(itemKind, fromPath, written, opts)
|
||||
if (await flushDraftDelete(itemKind, fromPath, opts)) return
|
||||
// Only a moved item forces the point. The form stays editable across that
|
||||
// request, so an edit made then parks a write behind it: under the path the
|
||||
// item has left that write would strand a draft no editor is on, but under an
|
||||
// unchanged path it is the user's, and is meant to stand and read dirty.
|
||||
if (savedPath === fromPath) return
|
||||
for (let attempt = 0; attempt < 2; attempt++) {
|
||||
// The form stays editable across the delete's own request, so a keystroke made
|
||||
// then queues a write behind it. Under an unchanged path that write is the
|
||||
// user's and is meant to stand; under a path the item has left it would put the
|
||||
// draft back where no editor is, so the old key stops accepting writes for the
|
||||
// length of the delete — which is what makes one attempt final. The edit stays
|
||||
// in the form, and could not have followed the rename in either case.
|
||||
const moved = savedPath !== fromPath
|
||||
if (moved) UserDraft.stopSync(itemKind, fromPath, opts)
|
||||
try {
|
||||
// Sent before this resolves, not left on the keystroke debounce: callers report
|
||||
// the write and remount on it, and both read a cell this has to have finished
|
||||
// resolving — including through a frame with a draft store of its own.
|
||||
UserDraft.discard(itemKind, fromPath, written, opts)
|
||||
if (await flushDraftDelete(itemKind, fromPath, opts)) return
|
||||
} finally {
|
||||
if (moved) UserDraft.restartSync(itemKind, fromPath, opts)
|
||||
}
|
||||
if (!moved) return
|
||||
// A rename whose delete could not land — a conflict, or a failed request; no
|
||||
// number of retries changes either. What is left is a draft-only item at a path
|
||||
// nothing is editing. Callers still follow the rename, since the item really is
|
||||
// at `savedPath` and leaving them behind would strand the editor too, so this is
|
||||
// the only thing that says the leftover is there to be discarded.
|
||||
sendUserToast(`Saved, but the draft left at ${fromPath} could not be cleared`, true)
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -1,6 +1,8 @@
|
||||
import { describe, it, expect, beforeEach, vi } from 'vitest'
|
||||
|
||||
const discard = vi.fn()
|
||||
const { toast } = vi.hoisted(() => ({ toast: vi.fn() }))
|
||||
vi.mock('./utils', async (orig) => ({ ...((await orig()) as object), sendUserToast: toast }))
|
||||
// Whether the key ends up settled on the delete after a flush — false stands for a
|
||||
// form that keeps queueing writes behind it.
|
||||
let settles = true
|
||||
@@ -20,6 +22,7 @@ import { settleDraftAfterWrite, UserDraft } from './userDraft.svelte'
|
||||
|
||||
beforeEach(() => {
|
||||
settles = true
|
||||
toast.mockClear()
|
||||
discard.mockClear()
|
||||
vi.spyOn(UserDraft, 'discard').mockImplementation(discard as any)
|
||||
})
|
||||
@@ -65,14 +68,21 @@ describe('settleDraftAfterWrite', () => {
|
||||
expect(discard).toHaveBeenCalledWith('variable', 'u/me/a', sent, OPTS)
|
||||
})
|
||||
|
||||
// The form stays editable across the delete's own request, so an edit made then
|
||||
// parks a write that would put the draft back — under a path the item has left,
|
||||
// once the callers re-key. The delete is re-sent while that is the case, and
|
||||
// bounded, because a form being typed into can always add one more.
|
||||
it('re-sends the delete while the key will not settle on it, and gives up bounded', async () => {
|
||||
// A rename suspends the old key first, so nothing can queue behind the delete and
|
||||
// re-sending it could only repeat a request the server already refused. What is
|
||||
// left is a draft under a path the item has left, which has to be reported.
|
||||
it('sends the delete once on a rename and reports one that will not land', async () => {
|
||||
settles = false
|
||||
await settleDraftAfterWrite('variable', sent, { ...sent }, 'u/me/a', 'u/me/b', OPTS)
|
||||
expect(discard).toHaveBeenCalledTimes(3)
|
||||
expect(discard).toHaveBeenCalledTimes(1)
|
||||
expect(toast).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
// A create's form cell is the detached handle an empty path gets, wired to no
|
||||
// key: every request made for one is addressed to `/drafts/update/<kind>/`.
|
||||
it('settles nothing for a create', async () => {
|
||||
await settleDraftAfterWrite('variable', sent, undefined, '', 'u/me/a', OPTS)
|
||||
expect(discard).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
// A released handle reads as no cell at all. That is not somebody's newer edit —
|
||||
@@ -82,13 +92,14 @@ describe('settleDraftAfterWrite', () => {
|
||||
expect(discard).toHaveBeenCalledWith('variable', 'u/me/a', sent, OPTS)
|
||||
})
|
||||
|
||||
// Retrying resets the cell to what was written, so it may only chase a path the
|
||||
// item has left. On an unchanged path an edit made during the delete is the
|
||||
// user's, and re-sending would revert it.
|
||||
it('does not retry over an edit made on an unchanged path', async () => {
|
||||
// On an unchanged path an edit made during the delete is the user's: the key is
|
||||
// left accepting writes, and a draft standing there is the intended outcome
|
||||
// rather than something to report.
|
||||
it('leaves an edit made during the delete on an unchanged path', async () => {
|
||||
settles = false
|
||||
await settleDraftAfterWrite('variable', sent, { ...sent }, 'u/me/a', 'u/me/a', OPTS)
|
||||
expect(discard).toHaveBeenCalledTimes(1)
|
||||
expect(toast).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('sends it once when the key settles', async () => {
|
||||
|
||||
Reference in New Issue
Block a user