starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-10-13 08:02:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: preserve the path a draft value already carries

Browse Source 
Round-4 review found a P1 in the previous commit's own fix.

Writing the chosen name into the value was right for a script, whose `path` is its
name, and wrong for every other kind. A flow keeps the storage key in `path` and
its typed rename in `draft_path`, and the editor compares the two: equal means
"rename applied", so it deletes `draft_path` on the next autosave
(FlowBuilder.svelte, FlowEditorView.svelte:87). A chat edit of a named flow draft
therefore cost it its name — the lists fell back to `draft_<uuid>`, a later
name-addressed write forked a second draft, and the deploy card reported the key.

Both kinds want the same thing, so there is no split to get wrong: keep the path
the value already carries, and let only a create take one from the call. That is
all the original bug ever was — the addressed path was written over a field that
already held the right answer.

The deployed-item check moves back behind the drafts listing, where it is reached
only once something is staged under the name. Hoisting it ahead of the listing
saved a workspace-wide read on the common path, but it put the check on every call
and swallowing its failure let a `delete_workspace_item` or `deploy_workspace_item`
fall through to a namesake draft. The listing-failure salvage it was added for
stays, inside the catch, where a failed probe only means the write is still
refused.

Tests: the flow case now pins `path` as the storage key, and the suite resets the
three deployed-existence mocks per test — `clearAllMocks` keeps implementations,
which is what made one test's leftovers answer for the next.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Guilhem Lemouel
2026-09-25 16:54:19 +02:00
co-authored by Claude Opus 5
parent b5acdd9181
commit 7ce85a4fde
3 changed files with 39 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files
frontend/src/lib/components/copilot/chat/global/core.test.ts
+8 -1
View File
@@ -521,6 +521,11 @@ describe('global AI tools', () => {
failingReads.clear()
clearGlobalDrafts(WORKSPACE)
vi.clearAllMocks()
// `clearAllMocks` keeps implementations, so a test that says "this item is deployed"
// would answer for every later test too, making them order-dependent.
vi.mocked(ScriptService.existsScriptByPath).mockImplementation(async () => false)
vi.mocked(FlowService.existsFlowByPath).mockImplementation(async () => false)
vi.mocked(AppService.existsApp).mockImplementation(async () => false)
})
it('defaults the datatable instruction subject to the TypeScript SQL SDK', async () => {
@@ -3245,7 +3250,9 @@ describe('global AI tools', () => {
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')
// `path` stays the storage key: the flow editor compares a typed rename against it
// and drops `draft_path` when they agree, which would lose the name entirely.
expect(draft?.path).toBe('u/admin/draft_f1')
})
// Same private-owner read path as schedules, for the trigger drawer kinds.
frontend/src/lib/components/copilot/chat/global/core.ts
+23 -23
View File
@@ -5066,12 +5066,12 @@ function stripBackendMetadata<T extends DraftConfig>(value: T): T {
function mergeDraftConfig<T extends DraftConfig>(
base: T | undefined,
overrides: DraftConfig,
chosenName: string
valuePath: string
): T {
return {
...(base ? stripBackendMetadata(base) : {}),
...structuredClone(overrides),
path: chosenName
path: valuePath
} as unknown as T
}
@@ -5366,9 +5366,9 @@ function finishDraftWrite(
type WriteSpec<T, A> = {
probe: (workspace: string, path: string) => Promise<boolean>
fetchDeployed: (workspace: string, path: string) => Promise<T>
/** `chosenName` is the name the draft deploys under, which is the only path a value
* may carry — never the key it is stored at. */
buildDraft: (base: T | undefined, args: A, chosenName: string) => T | Promise<T>
/** `valuePath` is what belongs in the value's own `path` for this kind — a script's
* name, every other kind's storage key. Never the path the call named. */
buildDraft: (base: T | undefined, args: A, valuePath: string) => T | Promise<T>
}
async function writeDraft<T, A>(
@@ -5396,15 +5396,15 @@ async function writeDraft<T, A>(
existed = true
}
// The name a base already carries wins over the path the call named: a draft is
// addressable by its storage key, and writing that key back into the value would
// rename the item to it — the deployer deploys at the path inside the value. Only a
// create (no base) takes its name from the call.
const itemKind = itemKindFor(type, opts.triggerKind)
const chosenName =
(base !== undefined && itemKind ? chosenDraftName(itemKind, base) : undefined) ?? target.path
// The path the value already carries, whatever it means for this kind: a script's is its
// name, a flow's is the storage key its typed rename is compared against. Either way the
// path the call named is not it — a draft is addressable by its storage key, and writing
// that key over a script's `path` renames the script to it, while writing it over a
// flow's makes the rename look applied and the editor then drops `draft_path`. Only a
// create, with no base to preserve, takes its path from the call.
const valuePath = (base as { path?: string } | undefined)?.path || target.path
const draft = await spec.buildDraft(base, args, chosenName)
const draft = await spec.buildDraft(base, args, valuePath)
const result = await persistGlobalDraft(workspace, type, target, draft, {
triggerKind: opts.triggerKind,
@@ -5428,17 +5428,17 @@ const SCRIPT_SPEC: WriteSpec<NewScript, ScriptDraftArgs> = {
const existing = await ScriptService.getScriptByPath({ workspace, path })
return { ...(existing as unknown as NewScript), parent_hash: existing.hash }
},
buildDraft: async (base, args, chosenName) => {
buildDraft: async (base, args, valuePath) => {
const draft: NewScript = base
? {
...structuredClone(base),
path: chosenName,
path: valuePath,
summary: args.summary ?? base.summary,
content: args.content,
language: args.language
}
: {
path: chosenName,
path: valuePath,
summary: args.summary ?? '',
description: '',
content: args.content,
@@ -5498,7 +5498,7 @@ const FLOW_STRUCTURAL_VALUE_KEYS = new Set<string>(EDITABLE_FLOW_STRUCTURAL_KEYS
const FLOW_SPEC: WriteSpec<Flow, FlowDraftArgs> = {
probe: (workspace, path) => FlowService.existsFlowByPath({ workspace, path }),
fetchDeployed: (workspace, path) => FlowService.getFlowByPath({ workspace, path }),
buildDraft: (base, args, chosenName) => {
buildDraft: (base, args, valuePath) => {
const value = structuredClone(args.flow.value)
if (args.flow.groups !== undefined && args.flow.groups !== null) {
value.groups = structuredClone(args.flow.groups)
@@ -5516,14 +5516,14 @@ const FLOW_SPEC: WriteSpec<Flow, FlowDraftArgs> = {
return base
? {
...structuredClone(base),
path: chosenName,
path: valuePath,
summary: args.summary ?? base.summary,
description: args.description ?? base.description,
value,
schema: args.flow.schema ?? base.schema
}
: {
path: chosenName,
path: valuePath,
summary: args.summary ?? '',
description: args.description ?? '',
value,
@@ -5544,11 +5544,11 @@ const SCHEDULE_SPEC: WriteSpec<ScheduleDraftConfig, NewSchedule & { override?: b
probe: (workspace, path) => ScheduleService.existsSchedule({ workspace, path }),
fetchDeployed: async (workspace, path) =>
(await ScheduleService.getSchedule({ workspace, path })) as ScheduleDraftConfig,
buildDraft: (base, args, chosenName) => {
buildDraft: (base, args, valuePath) => {
// `override` is a tool-only conflict-resolution flag, not schedule config —
// strip it so mergeDraftConfig doesn't clone it into the persisted draft.
const { override: _override, ...config } = args
return mergeDraftConfig<ScheduleDraftConfig>(base, config as DraftConfig, chosenName)
return mergeDraftConfig<ScheduleDraftConfig>(base, config as DraftConfig, valuePath)
}
}
@@ -5565,8 +5565,8 @@ function triggerWriteSpec(kind: TriggerKind): WriteSpec<TriggerDraftConfig, Trig
probe: (workspace, path) => service.exists({ workspace, path }),
fetchDeployed: async (workspace, path) =>
(await service.get({ workspace, path })) as TriggerDraftConfig,
buildDraft: (base, config, chosenName) => {
const draft = mergeDraftConfig<TriggerDraftConfig>(base, config, chosenName)
buildDraft: (base, config, valuePath) => {
const draft = mergeDraftConfig<TriggerDraftConfig>(base, config, valuePath)
if (kind === 'email') {
// workspaced_local_part maps to a NOT NULL column but is optional in the
// tool schema. Default on the merged draft (not the incoming config) so an
frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts
+8 -14
View File
@@ -375,21 +375,7 @@ 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.
//
// A failed probe falls through to the listing rather than out of the call: the listing
// below degrades on the same class of transient failure, and a read that addressed a
// deployed path made no such request at all before.
const exists = deployedExists[itemKind]
try {
if (exists && (await exists(workspace, path))) return path
} catch {
// fall through to the listing
}
let rows: Awaited<ReturnType<typeof DraftService.listDrafts>>
try {
rows = await DraftService.listDrafts({ workspace })
@@ -402,6 +388,9 @@ async function storagePathForChosenName(
// nothing the listing would have told a write either — refusing it would fail a
// write that has no ambiguity to resolve.
if (!opts.forWrite || !NAMEABLE_DRAFT_KINDS.has(itemKind)) return path
// A deployed item keeps its own path whatever is staged, so the listing was not
// needed after all. Only refuse when that cannot be established either.
if (exists && (await exists(workspace, path).catch(() => false))) return path
throw new Error(
`Could not load this workspace's drafts, so "${path}" cannot be matched to the draft ` +
`it may name: ${e instanceof Error ? e.message : String(e)}. Try again.`
@@ -417,6 +406,11 @@ 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. Consulted only once something is staged,
// so the common path costs no request and a failure here — which would leave a delete or
// a deploy pointing at a namesake draft — stops the call rather than being guessed past.
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