mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-08 08:04:25 +00:00
fix: open the gate on the edit itself, and stop the sweep at ownerless drafts
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ScVgqGpuyDMWdzVNPm7Q5f
This commit is contained in:
co-authored by
Claude Opus 5
parent
e586f2b8e4
commit
92ebe73494
@@ -136,8 +136,16 @@
|
||||
// to its form — the form is not on screen yet — but it would open the gate,
|
||||
// and the gating effect would then un-suspend the moment the fetch lands,
|
||||
// in time for the schema's materialized values to POST as a draft.
|
||||
onUserInput(() => {
|
||||
if (selected && selected in states) userEdited[selected] = true
|
||||
//
|
||||
// The same holds for the rest of the load: the schema arrives separately and
|
||||
// materializes on arrival, so until it settles a bare click is not enough to
|
||||
// call this an edit. `Path`, the labels and the description ARE editable
|
||||
// through that window though — they render above the schema form's skeleton
|
||||
// — so a real value event still opens the gate and keeps that edit.
|
||||
onUserInput((kind) => {
|
||||
if (!selected || !(selected in states)) return
|
||||
if (kind === 'precursor' && loadingSchema) return
|
||||
userEdited[selected] = true
|
||||
})
|
||||
|
||||
$effect(() => {
|
||||
|
||||
@@ -1,5 +1,22 @@
|
||||
import { onDestroy } from 'svelte'
|
||||
|
||||
/**
|
||||
* What kind of event opened the gate.
|
||||
*
|
||||
* - `value`: the user changed something — the event *is* the edit.
|
||||
* - `precursor`: a gesture that usually precedes an edit. Needed because a
|
||||
* custom component (a picker, a toggle built out of divs) writes its value
|
||||
* through Svelte state and fires no native value event at all, so waiting for
|
||||
* one would drop those edits. The cost is that a bare click counts too.
|
||||
*/
|
||||
export type UserInputKind = 'value' | 'precursor'
|
||||
|
||||
/** Events that ARE an edit. `drop` and `paste` matter on their own: text
|
||||
* dragged in from another application, or an assistive technology activating a
|
||||
* control, produce no pointer or key event in this document at all. */
|
||||
const VALUE_EVENTS = ['input', 'change', 'drop', 'paste'] as const
|
||||
const PRECURSOR_EVENTS = ['pointerdown', 'keydown'] as const
|
||||
|
||||
/**
|
||||
* A draft is supposed to record what the USER changed, but an editor built
|
||||
* from a schema writes into the value on its own: the form materializes a
|
||||
@@ -16,25 +33,30 @@ import { onDestroy } from 'svelte'
|
||||
* gating means for them — suspending the autosave, absorbing the settled value
|
||||
* into the deployed baseline, or both.
|
||||
*
|
||||
* `pointerdown` and `keydown` are the two events that precede every human
|
||||
* edit, and capture phase puts this ahead of the handler that writes the
|
||||
* value, so a gate opened here is already open by the time the edit lands.
|
||||
* Listening on the document rather than the editor's own subtree is
|
||||
* deliberate: pickers and modals render in portals outside it, and missing a
|
||||
* real edit would silently drop the user's work, while opening the gate too
|
||||
* eagerly only costs the phantom draft that existed before.
|
||||
* Capture phase puts this ahead of the handler that writes the value, so a gate
|
||||
* opened here is already open by the time the edit lands. Listening on the
|
||||
* document rather than the editor's own subtree is deliberate: pickers and
|
||||
* modals render in portals outside it, and missing a real edit would silently
|
||||
* drop the user's work, while opening the gate too eagerly only costs the
|
||||
* phantom draft that existed before.
|
||||
*
|
||||
* Registers for the lifetime of the calling component — call it during init.
|
||||
*/
|
||||
export function onUserInput(handle: () => void): void {
|
||||
export function onUserInput(handle: (kind: UserInputKind) => void): void {
|
||||
if (typeof document === 'undefined') return
|
||||
const onEvent = (e: Event) => {
|
||||
if (e.isTrusted) handle()
|
||||
const listeners: Array<[string, (e: Event) => void]> = []
|
||||
const register = (type: string, kind: UserInputKind) => {
|
||||
const onEvent = (e: Event) => {
|
||||
// A programmatic `dispatchEvent` is untrusted, which is what keeps the
|
||||
// form's own settling from opening the gate it is gated by.
|
||||
if (e.isTrusted) handle(kind)
|
||||
}
|
||||
document.addEventListener(type, onEvent, true)
|
||||
listeners.push([type, onEvent])
|
||||
}
|
||||
document.addEventListener('pointerdown', onEvent, true)
|
||||
document.addEventListener('keydown', onEvent, true)
|
||||
for (const type of VALUE_EVENTS) register(type, 'value')
|
||||
for (const type of PRECURSOR_EVENTS) register(type, 'precursor')
|
||||
onDestroy(() => {
|
||||
document.removeEventListener('pointerdown', onEvent, true)
|
||||
document.removeEventListener('keydown', onEvent, true)
|
||||
for (const [type, onEvent] of listeners) document.removeEventListener(type, onEvent, true)
|
||||
})
|
||||
}
|
||||
|
||||
@@ -150,12 +150,48 @@ describe('pruneMeaninglessDrafts', () => {
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
expect(sendUserToast).not.toHaveBeenCalled()
|
||||
discardDraft.mockResolvedValue({ success: true })
|
||||
// Sentinel unwritten, so the next mount retries the draft left behind.
|
||||
// A same-session retry sees the key as busy (a failed save leaves the
|
||||
// payload parked), attempts nothing — and must still not seal the pass.
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
expect(discardedPaths()).toEqual(['u/me/r'])
|
||||
// Once the failure clears, the draft left behind is finally retried.
|
||||
syncState = 'none'
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
expect(discardedPaths()).toEqual(['u/me/r', 'u/me/r'])
|
||||
})
|
||||
|
||||
it('never touches a legacy workspace-level row', async () => {
|
||||
listDrafts.mockResolvedValue([row({ legacy_draft: true })])
|
||||
getDraftDiffValues.mockResolvedValue(diff())
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
expect(discardDraft).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('only sweeps the kinds whose editors are gated', async () => {
|
||||
listDrafts.mockResolvedValue([
|
||||
row({ kind: 'script', path: 'u/me/s' }),
|
||||
row({ kind: 'flow', path: 'u/me/f' }),
|
||||
row({ kind: 'app', path: 'u/me/a' }),
|
||||
row({ kind: 'trigger_schedule', path: 'u/me/sched' }),
|
||||
row()
|
||||
])
|
||||
getDraftDiffValues.mockResolvedValue(diff())
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
expect(discardedPaths().sort()).toEqual(['u/me/r', 'u/me/sched'])
|
||||
// The expensive payload fetches are never made for the ungated kinds.
|
||||
expect(getDraftDiffValues).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
|
||||
it('leaves the pass open when a row was skipped as busy', async () => {
|
||||
liveDraft = true
|
||||
listDrafts.mockResolvedValue([row()])
|
||||
getDraftDiffValues.mockResolvedValue(diff())
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
liveDraft = false
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
expect(discardedPaths()).toEqual(['u/me/r'])
|
||||
})
|
||||
|
||||
it('runs once per workspace and user', async () => {
|
||||
listDrafts.mockResolvedValue([row()])
|
||||
getDraftDiffValues.mockResolvedValue(diff())
|
||||
|
||||
@@ -8,12 +8,19 @@
|
||||
* browser, after the localStorage→DB migration so anything it just uploaded is
|
||||
* swept too.
|
||||
*
|
||||
* Scoped to the kinds whose editors this gate covers. A script, flow or app
|
||||
* draft only ever came from an explicit edit, so there is no phantom to clear
|
||||
* there — and `getDraftDiffValues` would fetch each one's full deployed payload
|
||||
* at login to prove it.
|
||||
*
|
||||
* A draft is dropped only when the diff the user would be shown is empty: both
|
||||
* sides come from `getDraftDiffValues`, the same canonicalization the diff
|
||||
* drawer renders, compared with the same `draftValuesEqual` the editors use.
|
||||
* Anything that can't be established is left alone — a `draft_only` item (no
|
||||
* deployed counterpart, so discarding would destroy the item itself), a kind
|
||||
* with no diff support, a failed fetch.
|
||||
* with no diff support, a failed fetch, and the legacy workspace-level rows,
|
||||
* which belong to nobody, are admin-gated to migrate, and whose discard path
|
||||
* takes no `last_sync` and so could not be conditioned anyway.
|
||||
*
|
||||
* Deleting is the dangerous half, and the equality behind it is always stale:
|
||||
* it was read one round trip ago, and every candidate is read before any is
|
||||
@@ -36,6 +43,12 @@ import { invalidateWorkspaceDrafts } from './workspaceDrafts.svelte'
|
||||
|
||||
const SENTINEL_PREFIX = 'userdraft/pruned/v1/'
|
||||
|
||||
/** The editors whose forms are built from a schema, and so the only kinds that
|
||||
* could have banked a draft nobody wrote. */
|
||||
function isGatedKind(kind: UserDraftItemKind): boolean {
|
||||
return kind === 'resource' || kind.startsWith('trigger_')
|
||||
}
|
||||
|
||||
/** Overlay GETs are one round trip each and a cluttered workspace has dozens;
|
||||
* a small window keeps the sweep off the critical path of a fresh login. */
|
||||
const CONCURRENCY = 4
|
||||
@@ -46,7 +59,6 @@ const inFlight = new Set<string>()
|
||||
type Candidate = {
|
||||
kind: UserDraftItemKind
|
||||
path: string
|
||||
legacy: boolean
|
||||
/** The row's `created_at` as listed — the baseline the delete is conditioned on. */
|
||||
createdAt: string
|
||||
}
|
||||
@@ -106,15 +118,24 @@ export async function pruneMeaninglessDrafts(workspace: string, userKey: string)
|
||||
inFlight.add(sentinel)
|
||||
try {
|
||||
const rows = await DraftService.listDrafts({ workspace })
|
||||
// Anything left unresolved keeps the pass open: a row skipped as busy was
|
||||
// never judged, and one whose delete failed is still there. Sealing on
|
||||
// either would strand it — a later pass in the same session would skip it
|
||||
// again (a failed key reads as busy) and then seal with nothing attempted.
|
||||
let unresolved = 0
|
||||
const candidates: Candidate[] = rows
|
||||
// `draft_only` rows ARE the item; `mine` / `can_write` are the same
|
||||
// gate the discard endpoint enforces, so anything else would 403.
|
||||
.filter((r) => !r.draft_only && r.mine && r.can_write)
|
||||
.filter((r) => !busyLocally(workspace, r.kind, r.path))
|
||||
.filter((r) => isGatedKind(r.kind) && !r.legacy_draft)
|
||||
.filter((r) => {
|
||||
if (!busyLocally(workspace, r.kind, r.path)) return true
|
||||
unresolved++
|
||||
return false
|
||||
})
|
||||
.map((r) => ({
|
||||
kind: r.kind,
|
||||
path: r.path,
|
||||
legacy: r.legacy_draft,
|
||||
createdAt: r.created_at
|
||||
}))
|
||||
|
||||
@@ -124,27 +145,27 @@ export async function pruneMeaninglessDrafts(workspace: string, userKey: string)
|
||||
})
|
||||
|
||||
let discarded = 0
|
||||
let failed = 0
|
||||
for (const c of empty) {
|
||||
// Re-check: the reads above took a while, and the user may have opened
|
||||
// this item in the meantime.
|
||||
if (busyLocally(workspace, c.kind, c.path)) continue
|
||||
if (busyLocally(workspace, c.kind, c.path)) {
|
||||
unresolved++
|
||||
continue
|
||||
}
|
||||
const q = { workspace, itemKind: c.kind, path: c.path }
|
||||
UserDraftDbSyncer.recordRemoteSync(q, c.createdAt)
|
||||
const res = await discardDraft(c.kind, c.path, workspace, false, c.legacy, false)
|
||||
const res = await discardDraft(c.kind, c.path, workspace, false, false, false)
|
||||
// Neither outcome throws: the syncer swallows an HTTP failure into its
|
||||
// per-key state, and a delete refused for a moved row comes back as a
|
||||
// conflict. So `success` alone says nothing about whether the row went.
|
||||
if (!res.success || UserDraftDbSyncer.getState(q).state === 'failed') failed++
|
||||
if (!res.success || UserDraftDbSyncer.getState(q).state === 'failed') unresolved++
|
||||
else if (!UserDraftDbSyncer.getConflict(q).conflict) discarded++
|
||||
}
|
||||
if (discarded > 0) {
|
||||
invalidateWorkspaceDrafts(workspace)
|
||||
sendUserToast(`Cleared ${discarded} draft${discarded > 1 ? 's' : ''} that carried no changes`)
|
||||
}
|
||||
// Only once every deletion this pass attempted actually landed. A draft
|
||||
// left behind by a failed delete would otherwise never be revisited.
|
||||
if (failed === 0) {
|
||||
if (unresolved === 0) {
|
||||
try {
|
||||
localStorage.setItem(sentinel, new Date().toISOString())
|
||||
} catch {
|
||||
|
||||
Reference in New Issue
Block a user