mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-13 16:02:59 +00:00
fix: settle the deployed path before listing, and stop the drafts page guessing
Two findings from a local review round. `storagePathForChosenName` fetched the workspace-wide draft listing before the deployed-item check that can answer on its own. A path with nothing staged under it is the common case — every read of a deployed item — and it now costs one indexed `exists` instead of a listing carrying a per-row owner aggregate. A write also no longer refuses when the listing fails but something is deployed at exactly that path, which is the one case the name is unambiguous anyway. The drafts page resolved every listed row by chosen name, although the only case it needs help with is a live editor's staged rename. A legacy row (one with no owner) has no draft of this user at its path, so the resolution walked on to the listing and matched an unrelated draft staged under the same name — Discard then removed that one and left the clicked row. `liveGlobalDraftTarget` follows the editor rename alone, synchronously, so "Clear all" also stops issuing a request per row. Tests: the name rule gains the flow-shaped case, where the name lives in `draft_path` rather than `value.path`, and `retargetEditorItem` gains one case per branch. Three `exists` mocks move from a single-use value to a path-scoped implementation: resolution asks the same question the write's own probe does, so counting calls made the mock answer the wrong caller, and a test's unconsumed `getScriptByPath` then leaked into the next one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
fa52d79706
commit
c2067894c4
@@ -3209,6 +3209,32 @@ describe('global AI tools', () => {
|
||||
expect(draft?.content).toBe('export async function main() { return 2 }')
|
||||
})
|
||||
|
||||
// Same rule, the other branch of `chosenDraftName`: a flow parks its name in
|
||||
// `draft_path`, so the name and the addressed path are separate fields here.
|
||||
it('keeps a flow draft name when the write addresses it by its storage key', async () => {
|
||||
seedBackendDraft(
|
||||
'flow',
|
||||
'u/admin/draft_f1',
|
||||
{
|
||||
path: 'u/admin/draft_f1',
|
||||
draft_path: 'f/team/new_flow',
|
||||
summary: 'Staged under a new name',
|
||||
value: { modules: [] }
|
||||
},
|
||||
{ workspace: WORKSPACE }
|
||||
)
|
||||
|
||||
await callGlobalTool('write_flow', {
|
||||
path: 'u/admin/draft_f1',
|
||||
summary: 'Updated flow',
|
||||
modules: JSON.stringify([{ id: 'step', value: { type: 'identity' } }])
|
||||
})
|
||||
|
||||
const draft = getBackendDraft<any>('flow', 'u/admin/draft_f1', { workspace: WORKSPACE })
|
||||
expect(draft?.draft_path).toBe('f/team/new_flow')
|
||||
expect(draft?.path).toBe('f/team/new_flow')
|
||||
})
|
||||
|
||||
// Same private-owner read path as schedules, for the trigger drawer kinds.
|
||||
it('reads and deploys a trigger draft written by the chat', async () => {
|
||||
await callGlobalTool('write_trigger', {
|
||||
@@ -3438,7 +3464,9 @@ describe('global AI tools', () => {
|
||||
})
|
||||
|
||||
it('preserves existing script metadata and seeds freshness on first script write', async () => {
|
||||
vi.mocked(ScriptService.existsScriptByPath).mockResolvedValueOnce(true)
|
||||
vi.mocked(ScriptService.existsScriptByPath).mockImplementation(
|
||||
async ({ path }) => path === 'f/scripts/existing'
|
||||
)
|
||||
vi.mocked(ScriptService.getScriptByPath).mockResolvedValueOnce({
|
||||
path: 'f/scripts/existing',
|
||||
hash: 'deployed-hash',
|
||||
@@ -3866,7 +3894,9 @@ describe('global AI tools', () => {
|
||||
})
|
||||
|
||||
it('preserves existing flow metadata and seeds freshness on first flow write', async () => {
|
||||
vi.mocked(FlowService.existsFlowByPath).mockResolvedValueOnce(true)
|
||||
vi.mocked(FlowService.existsFlowByPath).mockImplementation(
|
||||
async ({ path }) => path === 'f/flows/existing'
|
||||
)
|
||||
vi.mocked(FlowService.getFlowLatestVersion).mockResolvedValueOnce({ id: 42 } as any)
|
||||
vi.mocked(FlowService.getFlowByPath).mockResolvedValueOnce({
|
||||
path: 'f/flows/existing',
|
||||
@@ -5474,7 +5504,9 @@ describe('global AI tools', () => {
|
||||
// that rewrites the draft's schema from scratch, or a draft read that drops it, unmarks
|
||||
// the field — and the form then takes the secret as a plain literal into the job's args.
|
||||
it('test_run_script keeps the password marking of the script it previews', async () => {
|
||||
vi.mocked(ScriptService.existsScriptByPath).mockResolvedValueOnce(true)
|
||||
vi.mocked(ScriptService.existsScriptByPath).mockImplementation(
|
||||
async ({ path }) => path === 'f/scripts/secretful'
|
||||
)
|
||||
vi.mocked(ScriptService.getScriptByPath).mockResolvedValueOnce({
|
||||
path: 'f/scripts/secretful',
|
||||
language: 'bun',
|
||||
|
||||
@@ -365,6 +365,13 @@ async function storagePathForChosenName(
|
||||
if (staged.some((s) => s.storagePath === storagePath)) return
|
||||
staged.push({ storagePath, summary })
|
||||
}
|
||||
// A deployed item keeps its own path: drafts staged under that name belong to other
|
||||
// items and are reached by their storage path. Settled before the listing, which is
|
||||
// workspace-wide — an indexed single-row check answers the common case, where the path
|
||||
// is simply where something is deployed, and a path that is unambiguous by this rule is
|
||||
// then never refused because the listing failed.
|
||||
const exists = deployedExists[itemKind]
|
||||
if (exists && (await exists(workspace, path))) return path
|
||||
let rows: Awaited<ReturnType<typeof DraftService.listDrafts>>
|
||||
try {
|
||||
rows = await DraftService.listDrafts({ workspace })
|
||||
@@ -388,10 +395,6 @@ async function storagePathForChosenName(
|
||||
}
|
||||
}
|
||||
if (staged.length === 0) return path
|
||||
// A deployed item keeps its own path: drafts staged under that name belong to other
|
||||
// items and are reached by their storage path.
|
||||
const exists = deployedExists[itemKind]
|
||||
if (exists && (await exists(workspace, path))) return path
|
||||
if (staged.length > 1) {
|
||||
// Refused rather than guessed: nothing checks a name for uniqueness before deploy.
|
||||
const listed = staged
|
||||
@@ -429,6 +432,25 @@ export function liveGlobalDraftStoragePath(
|
||||
return itemKind ? liveEditorStoragePath(workspace, itemKind, path) : path
|
||||
}
|
||||
|
||||
/**
|
||||
* The target for a caller that already holds a storage key and only needs the open editor's
|
||||
* staged rename followed — a row it listed, not a path someone named. Synchronous, and
|
||||
* deliberately without the name lookup: a legacy row (one with no owner) has no draft of
|
||||
* this user at its path, so resolving it by name would walk on and match an unrelated draft
|
||||
* staged under the same name.
|
||||
*/
|
||||
export function liveGlobalDraftTarget(
|
||||
workspace: string,
|
||||
type: WorkspaceItemType,
|
||||
path: string,
|
||||
triggerKind?: TriggerKind
|
||||
): ResolvedDraftTarget {
|
||||
const storagePath = liveGlobalDraftStoragePath(workspace, type, path, triggerKind)
|
||||
const itemKind = itemKindFor(type, triggerKind)
|
||||
const cell = itemKind ? UserDraft.get(itemKind, storagePath, { workspace }) : undefined
|
||||
return { path, storagePath, value: cell, fetched: cell !== undefined }
|
||||
}
|
||||
|
||||
/** The draft a call addresses: the path it named, and whether the call goes on to write. */
|
||||
export type DraftTarget = {
|
||||
type: WorkspaceItemType
|
||||
|
||||
@@ -793,6 +793,31 @@ describe('SessionPreviewTabs.select / close / setCollapsed', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('SessionPreviewTabs.retargetEditorItem', () => {
|
||||
it('follows an editor item saved under a new path in place', () => {
|
||||
const o = owner()
|
||||
o.open(scriptTarget)
|
||||
const id = o.tabs[0].id
|
||||
|
||||
o.retargetEditorItem({ kind: 'script', path: 'u/me/foo' }, { kind: 'script', path: 'f/x/foo' })
|
||||
|
||||
expect(o.tabs).toHaveLength(1)
|
||||
expect(o.tabs[0].id).toBe(id)
|
||||
expect(o.tabs[0].url).toBe('/scripts/edit/f/x/foo')
|
||||
})
|
||||
|
||||
it('closes the tab when another one already holds the destination', () => {
|
||||
const o = owner()
|
||||
o.open(scriptTarget)
|
||||
o.open({ type: 'item', item: { kind: 'script', path: 'f/x/foo', summary: '' } })
|
||||
expect(o.tabs).toHaveLength(2)
|
||||
|
||||
o.retargetEditorItem({ kind: 'script', path: 'u/me/foo' }, { kind: 'script', path: 'f/x/foo' })
|
||||
|
||||
expect(o.tabs.map((t) => t.url)).toEqual(['/scripts/edit/f/x/foo'])
|
||||
})
|
||||
})
|
||||
|
||||
describe('SessionPreviewTabs.reorder', () => {
|
||||
it('reorders tabs to the given id order and persists, keeping the active id', () => {
|
||||
const { adapter, persisted } = makeAdapter()
|
||||
|
||||
@@ -4,7 +4,7 @@
|
||||
clearGlobalDrafts,
|
||||
deleteGlobalDraft,
|
||||
listGlobalDrafts,
|
||||
resolveDraftTarget
|
||||
liveGlobalDraftTarget
|
||||
} from '$lib/components/copilot/chat/global/userDraftAdapter'
|
||||
import type { WorkspaceItem } from '$lib/components/copilot/chat/global/workspaceItems'
|
||||
import { goto } from '$lib/navigation'
|
||||
@@ -53,16 +53,12 @@
|
||||
// A live editor's draft is listed under its chosen name, so the key to delete has to be
|
||||
// resolved from it; a persisted row is already listed under its storage key.
|
||||
function draftTargetOf(item: WorkspaceItem) {
|
||||
return resolveDraftTarget($workspaceStore!, {
|
||||
type: item.type,
|
||||
path: item.path,
|
||||
triggerKind: item.triggerKind
|
||||
})
|
||||
return liveGlobalDraftTarget($workspaceStore!, item.type, item.path, item.triggerKind)
|
||||
}
|
||||
|
||||
async function deleteDraft(item: WorkspaceItem) {
|
||||
if (!$workspaceStore) return
|
||||
await deleteGlobalDraft($workspaceStore, item.type, await draftTargetOf(item), item.triggerKind)
|
||||
await deleteGlobalDraft($workspaceStore, item.type, draftTargetOf(item), item.triggerKind)
|
||||
refreshDrafts()
|
||||
}
|
||||
|
||||
@@ -73,12 +69,7 @@
|
||||
// Continue past a per-row failure so one bad delete doesn't strand the rest.
|
||||
for (const item of [...drafts]) {
|
||||
try {
|
||||
await deleteGlobalDraft(
|
||||
$workspaceStore,
|
||||
item.type,
|
||||
await draftTargetOf(item),
|
||||
item.triggerKind
|
||||
)
|
||||
await deleteGlobalDraft($workspaceStore, item.type, draftTargetOf(item), item.triggerKind)
|
||||
} catch (e) {
|
||||
console.error('Failed to clear draft', item.path, e)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user