mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-13 08:02:47 +00:00
fix: report where a script or flow draft deploys to
Review round 2 findings: - `deployedPath` was resolved in the app branch only, so deploying a script or flow by its storage path reported that path as the destination. The preview then reloaded the path whose draft the deploy had just removed, and the chat's modified-item entry stayed there. The shared deployer deploys at the path inside the draft, so read that path after the flush. - A script draft's chosen name is the value's own `path`, which the draft-to- item mapping did not read, so lists and reads showed only the storage path. - `force` means "deploy over a newer version of this item", which cannot answer whether a name clash is the same item. The deploy refuses again, and the message names discard_local_draft as the way out of a leftover draft. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
b8a3887bbc
commit
db44d36d9b
@@ -3316,6 +3316,55 @@ describe('global AI tools', () => {
|
||||
expect(ScriptService.createScript).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
// The shared deployer deploys a script at the path inside the draft, so the deployed
|
||||
// path is the chosen name — what the preview follows and the mask entry moves to.
|
||||
it('reports where a draft-only script landed, not the key it was stored at', async () => {
|
||||
seedBackendDraft('script', 'u/admin/draft_s1', {
|
||||
path: 'f/team/new_script',
|
||||
summary: 's',
|
||||
description: '',
|
||||
content: 'export async function main() { return 1 }',
|
||||
language: 'bun',
|
||||
kind: 'script',
|
||||
schema: {}
|
||||
})
|
||||
vi.mocked(ScriptService.existsScriptByPath).mockResolvedValue(false)
|
||||
// What the shared deployer reads to deploy at the draft's own path.
|
||||
vi.mocked(ScriptService.getScriptByPath).mockResolvedValue({
|
||||
hash: 'h1',
|
||||
draft: {
|
||||
path: 'f/team/new_script',
|
||||
summary: 's',
|
||||
content: 'export async function main() { return 1 }',
|
||||
language: 'bun',
|
||||
kind: 'script',
|
||||
schema: {}
|
||||
}
|
||||
} as any)
|
||||
const onDeployed = vi.fn()
|
||||
setDeployedInSessionHandler(onDeployed)
|
||||
try {
|
||||
await callGlobalTool('deploy_workspace_item', {
|
||||
type: 'script',
|
||||
path: 'u/admin/draft_s1'
|
||||
})
|
||||
expect(ScriptService.createScript).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
requestBody: expect.objectContaining({ path: 'f/team/new_script' })
|
||||
})
|
||||
)
|
||||
expect(onDeployed).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
kind: 'script',
|
||||
path: 'u/admin/draft_s1',
|
||||
deployedPath: 'f/team/new_script'
|
||||
})
|
||||
)
|
||||
} finally {
|
||||
setDeployedInSessionHandler(undefined)
|
||||
}
|
||||
})
|
||||
|
||||
it('deploys a stale script draft when force is set', async () => {
|
||||
seedStaleScriptDraft('f/scripts/stale', 'base-hash')
|
||||
mockDeployedScript('f/scripts/stale', 'new-hash')
|
||||
|
||||
@@ -226,6 +226,7 @@ import {
|
||||
getGlobalDraft,
|
||||
getGlobalDraftStoragePath,
|
||||
itemKindFor,
|
||||
chosenDraftName,
|
||||
listGlobalDrafts,
|
||||
loadDraftNames,
|
||||
persistGlobalDraft,
|
||||
@@ -8022,6 +8023,11 @@ async function deployDraft(
|
||||
type === 'flow'
|
||||
? !(await FlowService.existsFlowByPath({ workspace, path: storagePath }))
|
||||
: false
|
||||
// The shared deployer deploys at the path inside the draft and doesn't report it
|
||||
// back, so read it here — post-flush, since a rename may have been parked. This is
|
||||
// where a draft-only or renamed item lands, and what the callbacks below must name.
|
||||
deployedPath =
|
||||
chosenDraftName(type, await readGlobalDraftValue(workspace, type, storagePath)) ?? storagePath
|
||||
const result = await deployDraftToWorkspace(type, storagePath, workspace, {
|
||||
draftOnly,
|
||||
deploymentMessage
|
||||
@@ -8218,13 +8224,15 @@ async function deployDraft(
|
||||
const targetExists = await AppService.existsApp({ workspace, path: targetPath })
|
||||
// A draft of a deployed app is stored at that app's path, so a target elsewhere that
|
||||
// already exists is another app: nothing checks a chosen name before deploy.
|
||||
// `force` is the way through for the one case this cannot tell apart: a deploy
|
||||
// that created the app and then failed to clean up its draft, retried.
|
||||
if (targetExists && targetPath !== storagePath && !force) {
|
||||
// `force` means "deploy over a newer version of this item" and cannot answer
|
||||
// this: the app at the target is a different item, so there is nothing to
|
||||
// overwrite on purpose. Discarding is the way out of the one ambiguous case,
|
||||
// a deploy that created the app and then failed to remove its draft.
|
||||
if (targetExists && targetPath !== storagePath) {
|
||||
throw new Error(
|
||||
`Cannot deploy app "${path}" to "${targetPath}": another app is already deployed there. ` +
|
||||
`Ask the user for a different path for this draft, or pass force: true if this draft ` +
|
||||
`is what is deployed there.`
|
||||
`Ask the user for a different path for this draft. If instead this draft is what is ` +
|
||||
`already deployed there, discard_local_draft removes the leftover draft.`
|
||||
)
|
||||
}
|
||||
if (targetExists) {
|
||||
|
||||
@@ -134,10 +134,10 @@ function scriptDraftToWorkspaceItem(path: string, draft: NewScript): WorkspaceIt
|
||||
return {
|
||||
type: 'script',
|
||||
path,
|
||||
// The session editor parks a rename in the draft's `draft_path` (see
|
||||
// sessionDraftCodecs.ts); surface it so lists/pickers show the friendly
|
||||
// name instead of the `draft_<uuid>` storage key.
|
||||
draftPath: (draft as NewScript & { draft_path?: string }).draft_path,
|
||||
// A script's chosen name is the value's own `path` — the editor binds the Path
|
||||
// widget to it — so a rename or a `draft_<uuid>` storage key shows up there, and
|
||||
// the session editor may also park one in `draft_path`.
|
||||
draftPath: chosenDraftName('script', draft) ?? (draft as { draft_path?: string }).draft_path,
|
||||
summary: draft.summary,
|
||||
language: draft.language,
|
||||
value: draft.content,
|
||||
@@ -318,7 +318,7 @@ const draftNamesByWorkspace = new Map<string, DraftNames>()
|
||||
|
||||
// Scripts keep their chosen path in the value's own `path`; the other kinds in `draft_path`
|
||||
// (the same split listDrafts applies server-side).
|
||||
function chosenDraftName(itemKind: UserDraftItemKind, value: unknown): string | undefined {
|
||||
export function chosenDraftName(itemKind: UserDraftItemKind, value: unknown): string | undefined {
|
||||
const v = value as { path?: string; draft_path?: string } | null | undefined
|
||||
return (itemKind === 'script' ? v?.path : v?.draft_path) || undefined
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user