mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-06 08:01:35 +00:00
feat: sweep away existing drafts that carry no changes, once per workspace
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
9787270ad8
commit
cd51af9252
@@ -237,6 +237,9 @@ const DRAFT_COMPARE_IGNORED_FIELDS = [
|
||||
// Computed at read time from the parent folder; edited on the folder, not
|
||||
// here. `labels` itself IS editable and stays compared.
|
||||
'inherited_labels',
|
||||
// Fixed at creation and absent from the resource editor's draft shape, so
|
||||
// it only ever shows up on the deployed side of a comparison.
|
||||
'resource_type',
|
||||
// A resource's OAuth/linked-secret state: owned by the OAuth flow and the
|
||||
// variable it points at, never by the resource form.
|
||||
'is_oauth',
|
||||
|
||||
@@ -0,0 +1,111 @@
|
||||
import { describe, it, expect, beforeEach, vi } from 'vitest'
|
||||
|
||||
// The sweep DELETES drafts, so the guard that matters is which rows it picks.
|
||||
// Stub the two collaborators it decides from — the draft listing and the
|
||||
// per-kind diff — and assert on what it discards.
|
||||
const listDrafts = vi.fn()
|
||||
const getDraftDiffValues = vi.fn()
|
||||
const discardDraft = vi.fn(async () => ({ success: true }))
|
||||
|
||||
vi.mock('./gen', () => ({
|
||||
DraftService: { listDrafts: (...a: unknown[]) => listDrafts(...(a as [])) }
|
||||
}))
|
||||
vi.mock('./utils_draft_deploy', () => ({
|
||||
getDraftDiffValues: (...a: unknown[]) => getDraftDiffValues(...(a as [])),
|
||||
discardDraft: (...a: unknown[]) => discardDraft(...(a as []))
|
||||
}))
|
||||
vi.mock('./workspaceDrafts.svelte', () => ({ invalidateWorkspaceDrafts: vi.fn() }))
|
||||
vi.mock('./toast', () => ({ sendUserToast: vi.fn() }))
|
||||
vi.mock('./userDraftDbSyncer.svelte', () => ({ UserDraftDbSyncer: { save: vi.fn() } }))
|
||||
|
||||
import { pruneMeaninglessDrafts } from './userDraftPrune'
|
||||
|
||||
const row = (over: Record<string, unknown> = {}) => ({
|
||||
kind: 'resource',
|
||||
path: 'u/me/r',
|
||||
draft_only: false,
|
||||
legacy_draft: false,
|
||||
mine: true,
|
||||
can_write: true,
|
||||
created_at: '',
|
||||
...over
|
||||
})
|
||||
const diff = (over: Record<string, unknown> = {}) => ({
|
||||
deployed: { value: { host: 'h' } },
|
||||
draft: { value: { host: 'h' } },
|
||||
hasDraft: true,
|
||||
noDeployed: false,
|
||||
...over
|
||||
})
|
||||
const discardedPaths = () => discardDraft.mock.calls.map((c: any[]) => c[1])
|
||||
|
||||
beforeEach(() => {
|
||||
localStorage.clear()
|
||||
vi.clearAllMocks()
|
||||
discardDraft.mockResolvedValue({ success: true })
|
||||
})
|
||||
|
||||
describe('pruneMeaninglessDrafts', () => {
|
||||
it('discards a draft whose diff against the deployed value is empty', async () => {
|
||||
listDrafts.mockResolvedValue([row()])
|
||||
getDraftDiffValues.mockResolvedValue(diff())
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
expect(discardedPaths()).toEqual(['u/me/r'])
|
||||
})
|
||||
|
||||
it('keeps a draft that carries a real change', async () => {
|
||||
listDrafts.mockResolvedValue([row()])
|
||||
getDraftDiffValues.mockResolvedValue(diff({ draft: { value: { host: 'other' } } }))
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
expect(discardDraft).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('ignores the empty fields a moved-on schema added', async () => {
|
||||
listDrafts.mockResolvedValue([row()])
|
||||
getDraftDiffValues.mockResolvedValue(
|
||||
diff({ draft: { value: { host: 'h', port: '', ssl: false, tags: [] } } })
|
||||
)
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
expect(discardedPaths()).toEqual(['u/me/r'])
|
||||
})
|
||||
|
||||
it('never touches a draft-only item — the draft is the whole item', async () => {
|
||||
listDrafts.mockResolvedValue([row({ draft_only: true })])
|
||||
getDraftDiffValues.mockResolvedValue(diff())
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
expect(discardDraft).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('never touches another user’s row, or one it cannot write', async () => {
|
||||
listDrafts.mockResolvedValue([row({ mine: false }), row({ path: 'u/me/b', can_write: false })])
|
||||
getDraftDiffValues.mockResolvedValue(diff())
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
expect(discardDraft).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('leaves a draft alone when its diff cannot be fetched', async () => {
|
||||
listDrafts.mockResolvedValue([row()])
|
||||
getDraftDiffValues.mockRejectedValue(new Error('boom'))
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
expect(discardDraft).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('runs once per workspace and user', async () => {
|
||||
listDrafts.mockResolvedValue([row()])
|
||||
getDraftDiffValues.mockResolvedValue(diff())
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
expect(discardDraft).toHaveBeenCalledTimes(1)
|
||||
await pruneMeaninglessDrafts('other', 'me@x.dev')
|
||||
expect(discardDraft).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
|
||||
it('retries next mount when the listing failed', async () => {
|
||||
listDrafts.mockRejectedValueOnce(new Error('offline'))
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
listDrafts.mockResolvedValue([row()])
|
||||
getDraftDiffValues.mockResolvedValue(diff())
|
||||
await pruneMeaninglessDrafts('main', 'me@x.dev')
|
||||
expect(discardedPaths()).toEqual(['u/me/r'])
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,122 @@
|
||||
/**
|
||||
* One-off sweep that drops drafts carrying no changes.
|
||||
*
|
||||
* Before the draft comparison learned to ignore empty schema-added fields and
|
||||
* server-managed metadata (see `normalizeDraftForCompare`), merely opening an
|
||||
* item whose schema had moved on saved a draft — workspaces accumulated dozens
|
||||
* that nobody wrote. New ones no longer appear; the ones already stored need
|
||||
* this pass to clear. Runs once per (workspace, user) per browser, after the
|
||||
* localStorage→DB migration so anything it just uploaded is swept too.
|
||||
*
|
||||
* 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.
|
||||
*/
|
||||
|
||||
import { DraftService } from './gen'
|
||||
import type { UserDraftItemKind } from './gen'
|
||||
import { sendUserToast } from './toast'
|
||||
import { draftValuesEqual } from './userDraft.svelte'
|
||||
import { discardDraft, getDraftDiffValues } from './utils_draft_deploy'
|
||||
import { invalidateWorkspaceDrafts } from './workspaceDrafts.svelte'
|
||||
|
||||
const SENTINEL_PREFIX = 'userdraft/pruned/v1/'
|
||||
|
||||
/** 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
|
||||
|
||||
/** Guards against the layout effect firing again before the sentinel lands. */
|
||||
const inFlight = new Set<string>()
|
||||
|
||||
type Candidate = {
|
||||
kind: UserDraftItemKind
|
||||
path: string
|
||||
legacy: boolean
|
||||
}
|
||||
|
||||
async function carriesNoChanges(workspace: string, { kind, path }: Candidate): Promise<boolean> {
|
||||
try {
|
||||
const { deployed, draft, hasDraft, noDeployed } = await getDraftDiffValues(
|
||||
kind,
|
||||
path,
|
||||
workspace
|
||||
)
|
||||
// `hasDraft` false means the overlay had no draft row and the item's own
|
||||
// value stood in for the draft side — there is nothing to discard, and the
|
||||
// two sides would compare equal by construction.
|
||||
if (!hasDraft || noDeployed) return false
|
||||
return draftValuesEqual(draft, deployed)
|
||||
} catch {
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
async function mapWithLimit<T, R>(
|
||||
items: T[],
|
||||
limit: number,
|
||||
fn: (item: T) => Promise<R>
|
||||
): Promise<R[]> {
|
||||
const out = new Array<R>(items.length)
|
||||
let next = 0
|
||||
await Promise.all(
|
||||
Array.from({ length: Math.min(limit, items.length) }, async () => {
|
||||
while (next < items.length) {
|
||||
const i = next++
|
||||
out[i] = await fn(items[i])
|
||||
}
|
||||
})
|
||||
)
|
||||
return out
|
||||
}
|
||||
|
||||
export async function pruneMeaninglessDrafts(workspace: string, userKey: string): Promise<void> {
|
||||
if (typeof localStorage === 'undefined') return
|
||||
const sentinel = `${SENTINEL_PREFIX}${workspace}/${userKey}`
|
||||
if (inFlight.has(sentinel)) return
|
||||
try {
|
||||
if (localStorage.getItem(sentinel)) return
|
||||
} catch {
|
||||
// Storage unavailable (private mode): the sweep can't record that it ran,
|
||||
// and re-running it on every mount would cost an overlay GET per draft.
|
||||
return
|
||||
}
|
||||
inFlight.add(sentinel)
|
||||
try {
|
||||
const rows = await DraftService.listDrafts({ workspace })
|
||||
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)
|
||||
.map((r) => ({ kind: r.kind, path: r.path, legacy: r.legacy_draft }))
|
||||
|
||||
const empty: Candidate[] = []
|
||||
await mapWithLimit(candidates, CONCURRENCY, async (c) => {
|
||||
if (await carriesNoChanges(workspace, c)) empty.push(c)
|
||||
})
|
||||
|
||||
let discarded = 0
|
||||
for (const c of empty) {
|
||||
const res = await discardDraft(c.kind, c.path, workspace, false, c.legacy, false)
|
||||
if (res.success) discarded++
|
||||
}
|
||||
if (discarded > 0) {
|
||||
invalidateWorkspaceDrafts(workspace)
|
||||
sendUserToast(`Cleared ${discarded} draft${discarded > 1 ? 's' : ''} that carried no changes`)
|
||||
}
|
||||
// Only after a completed pass: a run that threw retries on the next mount.
|
||||
try {
|
||||
localStorage.setItem(sentinel, new Date().toISOString())
|
||||
} catch {
|
||||
// Nothing to do — the pass is idempotent, it just runs again.
|
||||
}
|
||||
} catch {
|
||||
// Fire-and-forget from the layout: a workspace whose draft list can't be
|
||||
// read is left exactly as it was.
|
||||
} finally {
|
||||
inFlight.delete(sentinel)
|
||||
}
|
||||
}
|
||||
@@ -74,6 +74,7 @@
|
||||
import { createUsageResources, registerUsageResources } from '$lib/usage.svelte'
|
||||
import { purgeLegacyUserDrafts } from '$lib/userDraftLegacyMigration'
|
||||
import { migrateUserDraftsToDb } from '$lib/userDraftDbMigration'
|
||||
import { pruneMeaninglessDrafts } from '$lib/userDraftPrune'
|
||||
import DraftMigrationErrorModal from '$lib/components/DraftMigrationErrorModal.svelte'
|
||||
import { onDestroy, setContext, untrack } from 'svelte'
|
||||
import { base } from '$app/paths'
|
||||
@@ -787,12 +788,16 @@
|
||||
// drafts). `migrateUserDraftsToDb` then pushes the workspace-scoped
|
||||
// `userdraft/w/{ws}/{kind}/{path}` keys — written by the editor with the
|
||||
// correct workspace — onto the server-side draft table, clearing LS on
|
||||
// success.
|
||||
// success. `pruneMeaninglessDrafts` then clears the drafts an older, stricter
|
||||
// comparison saved for changes nobody made; it runs after the upload so the
|
||||
// entries that just landed are swept in the same pass.
|
||||
$effect(() => {
|
||||
if ($workspaceStore && $userStore) {
|
||||
const ws = $workspaceStore
|
||||
const email = $userStore?.email
|
||||
if (ws && email) {
|
||||
untrack(() => {
|
||||
purgeLegacyUserDrafts()
|
||||
void migrateUserDraftsToDb()
|
||||
void migrateUserDraftsToDb().then(() => pruneMeaninglessDrafts(ws, email))
|
||||
})
|
||||
}
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user