mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-12 16:02:56 +00:00
refactor: let a resolved target carry only the address
Third CI round, third P1 from the same source. `ResolvedDraftTarget` carried the draft's value alongside its key, so that a body reading the draft it just resolved would not fetch twice. Every reader that merges onto that value can be stale by the time it runs: a tool awaits other work before writing, and the user can edit, save and close the editor in that window. The first two rounds patched `readGlobalDraftValue` — once for the open editor, once for the editor that closed. Codex then found the same bug in the next reader along: `write_flow` restores `inline_script.<id>` placeholders through `getGlobalDraft`, so a body saved during the wait came back as the old code. Worse than before the patches, since the metadata around it was now preserved: the user would see their settings survive and their code silently revert. So the value is gone from the type. A resolved target is a path and a storage key, nothing else, and the two readers below it fetch: the cell if an editor holds one, else the backend. The cost is one request on calls that used to answer from the carried value; three P1s came out of saving it. `addressedDisplayPath` named the item from that value; it now asks the open editor, which answers synchronously and is the only place a name can be fresher than the stored draft anyway. Tests: the placeholder race Codex described, which fails against the previous commit's code. Also drops a comment line the `fresh` removal left restating the sentence above it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
45c0a35981
commit
5d1b20fb62
@@ -5569,6 +5569,67 @@ describe('global AI tools', () => {
|
||||
).resolves.toBe(code)
|
||||
})
|
||||
|
||||
// A tool awaits other work between the dispatch resolving its target and its write —
|
||||
// `write_flow` waits on the provider catalog. If the user edits an inline script and saves
|
||||
// in that window, restoring an `inline_script.<id>` placeholder from anything the dispatch
|
||||
// read would put the old body back.
|
||||
it('restores an inline script from the current draft, not the one resolution saw', async () => {
|
||||
const path = 'u/admin/draft_inline_race'
|
||||
const flowWith = (code: string) => ({
|
||||
path,
|
||||
draft_path: 'f/team/raced_flow',
|
||||
summary: 'Raced',
|
||||
value: {
|
||||
modules: [
|
||||
{
|
||||
id: 'call_api',
|
||||
value: { type: 'rawscript', language: 'bun', content: code, input_transforms: {} }
|
||||
}
|
||||
]
|
||||
}
|
||||
})
|
||||
seedBackendDraft('flow', path, flowWith('export async function main() { return 1 }'), {
|
||||
workspace: WORKSPACE
|
||||
})
|
||||
|
||||
const args = {
|
||||
path,
|
||||
summary: 'Reordered',
|
||||
modules: JSON.stringify([
|
||||
{
|
||||
id: 'call_api',
|
||||
value: {
|
||||
type: 'rawscript',
|
||||
language: 'bun',
|
||||
content: 'inline_script.call_api',
|
||||
input_transforms: {}
|
||||
}
|
||||
}
|
||||
])
|
||||
}
|
||||
const tool = getGlobalTool('write_flow')
|
||||
const target = await tool.draftTarget?.({ args, workspace: WORKSPACE })
|
||||
|
||||
// The user saves a new body while the tool is still awaiting.
|
||||
seedBackendDraft('flow', path, flowWith('export async function main() { return 2 }'), {
|
||||
workspace: WORKSPACE
|
||||
})
|
||||
|
||||
await tool.fn({
|
||||
args,
|
||||
workspace: WORKSPACE,
|
||||
helpers: {},
|
||||
toolCallbacks,
|
||||
toolId: 'test-race',
|
||||
target
|
||||
})
|
||||
|
||||
const saved = getBackendDraft<any>('flow', path, { workspace: WORKSPACE })
|
||||
expect(saved?.value?.modules?.[0]?.value?.content).toBe(
|
||||
'export async function main() { return 2 }'
|
||||
)
|
||||
})
|
||||
|
||||
it('writes flows with flow-mode arguments and reads compact flow value', async () => {
|
||||
const writeResult = JSON.parse(
|
||||
await callGlobalTool('write_flow', {
|
||||
|
||||
@@ -248,6 +248,7 @@ import {
|
||||
itemKindFor,
|
||||
chosenDraftName,
|
||||
listGlobalDrafts,
|
||||
liveGlobalDraftDisplayPath,
|
||||
liveGlobalDraftStoragePath,
|
||||
type DraftTarget,
|
||||
readGlobalDraftValue,
|
||||
@@ -4808,14 +4809,16 @@ function draftTarget(
|
||||
/** The name the addressed draft deploys under, known before the write from the value the
|
||||
* dispatch already read. Falls back to the addressed path when no draft was found there. */
|
||||
function addressedDisplayPath(
|
||||
workspace: string,
|
||||
target: ResolvedDraftTarget,
|
||||
type: WorkspaceItemType,
|
||||
triggerKind?: TriggerKind
|
||||
): string {
|
||||
const itemKind = itemKindFor(type, triggerKind)
|
||||
const name =
|
||||
itemKind && target.value !== undefined ? chosenDraftName(itemKind, target.value) : undefined
|
||||
return name ?? target.path
|
||||
// From the open editor, which holds the freshest name and answers without a request. A
|
||||
// draft no editor holds falls back to the path the call named; the write's own result
|
||||
// names it properly once the value has been read.
|
||||
const live = liveGlobalDraftDisplayPath(workspace, type, target.storagePath, triggerKind)
|
||||
return live === target.storagePath ? target.path : live
|
||||
}
|
||||
|
||||
/** The draft this call addresses. Failing here beats writing to a path that was never
|
||||
@@ -5521,7 +5524,7 @@ async function writeDraft<T, A>(
|
||||
): Promise<string> {
|
||||
const { workspace } = ctx
|
||||
const target = draftTargetOf(ctx)
|
||||
startDraftWrite(ctx, type, addressedDisplayPath(target, type, opts.triggerKind))
|
||||
startDraftWrite(ctx, type, addressedDisplayPath(workspace, target, type, opts.triggerKind))
|
||||
|
||||
const existingDraft = await readGlobalDraftValue<T>(workspace, type, target, {
|
||||
triggerKind: opts.triggerKind
|
||||
@@ -7006,7 +7009,7 @@ async function writeAppFile(
|
||||
assertNotGeneratedAppFile(target.filePath)
|
||||
|
||||
const draft = draftTargetOf(ctx)
|
||||
const appName = addressedDisplayPath(draft, 'app')
|
||||
const appName = addressedDisplayPath(workspace, draft, 'app')
|
||||
toolCallbacks.setToolStatus(toolId, {
|
||||
content: `Writing ${target.filePath} to app "${appName}"...`
|
||||
})
|
||||
@@ -7040,7 +7043,7 @@ async function deleteAppFile(
|
||||
assertNotGeneratedAppFile(target.filePath)
|
||||
|
||||
const draft = draftTargetOf(ctx)
|
||||
const appName = addressedDisplayPath(draft, 'app')
|
||||
const appName = addressedDisplayPath(workspace, draft, 'app')
|
||||
toolCallbacks.setToolStatus(toolId, {
|
||||
content: `Deleting ${target.filePath} from app "${appName}"...`
|
||||
})
|
||||
@@ -7087,7 +7090,7 @@ async function patchAppFile(
|
||||
}
|
||||
|
||||
const draft = draftTargetOf(ctx)
|
||||
const appName = addressedDisplayPath(draft, 'app')
|
||||
const appName = addressedDisplayPath(workspace, draft, 'app')
|
||||
toolCallbacks.setToolStatus(toolId, {
|
||||
content: `Patching ${target.filePath} in app "${appName}"...`
|
||||
})
|
||||
@@ -7162,7 +7165,7 @@ async function writeAppRunnable(
|
||||
const { workspace, toolId, toolCallbacks } = ctx
|
||||
const { key, runnable: input } = args
|
||||
const target = draftTargetOf(ctx)
|
||||
const appName = addressedDisplayPath(target, 'app')
|
||||
const appName = addressedDisplayPath(workspace, target, 'app')
|
||||
toolCallbacks.setToolStatus(toolId, {
|
||||
content: `Writing runnable "${key}" to app "${appName}"...`
|
||||
})
|
||||
@@ -7308,7 +7311,7 @@ async function deleteAppRunnable(
|
||||
const { workspace, toolId, toolCallbacks } = ctx
|
||||
const { key } = args
|
||||
const target = draftTargetOf(ctx)
|
||||
const appName = addressedDisplayPath(target, 'app')
|
||||
const appName = addressedDisplayPath(workspace, target, 'app')
|
||||
toolCallbacks.setToolStatus(toolId, {
|
||||
content: `Removing runnable "${key}" from app "${appName}"...`
|
||||
})
|
||||
@@ -8598,7 +8601,6 @@ async function deployDraft(
|
||||
// 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.
|
||||
// Read after the flush above, which may have persisted a rename parked in the editor.
|
||||
deployedPath =
|
||||
chosenDraftName(type, await readGlobalDraftValue(workspace, type, target)) ?? storagePath
|
||||
const result = await deployDraftToWorkspace(type, storagePath, workspace, {
|
||||
|
||||
@@ -468,6 +468,20 @@ function liveEditorStoragePath(
|
||||
return path
|
||||
}
|
||||
|
||||
/** The name to show for a draft stored at `storagePath`, from the open editor alone — the
|
||||
* staged rename it holds, else the key itself. Synchronous, so a status line can name the
|
||||
* item without a request; a caller that needs the name of a draft no editor holds reads it
|
||||
* from the value it already has. */
|
||||
export function liveGlobalDraftDisplayPath(
|
||||
workspace: string,
|
||||
type: WorkspaceItemType,
|
||||
storagePath: string,
|
||||
triggerKind?: TriggerKind
|
||||
): string {
|
||||
const itemKind = itemKindFor(type, triggerKind)
|
||||
return itemKind ? liveDisplayPath(workspace, itemKind, storagePath).displayPath : storagePath
|
||||
}
|
||||
|
||||
/** Follows the open editor's staged rename only, for callers that already hold a storage
|
||||
* path. A path that may be a draft's chosen name needs `resolveDraftTarget`. */
|
||||
export function liveGlobalDraftStoragePath(
|
||||
@@ -493,10 +507,7 @@ export function liveGlobalDraftTarget(
|
||||
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 }
|
||||
return { path, storagePath: liveGlobalDraftStoragePath(workspace, type, path, triggerKind) }
|
||||
}
|
||||
|
||||
/** The draft a call addresses: the path it named, and whether the call goes on to write. */
|
||||
@@ -523,16 +534,9 @@ export async function resolveDraftTarget(
|
||||
): Promise<ResolvedDraftTarget> {
|
||||
const { type, path, triggerKind, forWrite } = target
|
||||
const itemKind = itemKindFor(type, triggerKind)
|
||||
if (!itemKind) return { path, storagePath: path, fetched: false }
|
||||
if (!itemKind) return { path, storagePath: path }
|
||||
const resolved = await resolveDraft(workspace, itemKind, path, { forWrite })
|
||||
const cell = UserDraft.get(itemKind, resolved.storagePath, { workspace })
|
||||
return {
|
||||
path,
|
||||
storagePath: resolved.storagePath,
|
||||
value: cell ?? resolved.value,
|
||||
// Only the path it probed was fetched; a name resolved elsewhere was not.
|
||||
fetched: cell !== undefined || resolved.storagePath === path
|
||||
}
|
||||
return { path, storagePath: resolved.storagePath }
|
||||
}
|
||||
|
||||
// Current user's persisted draft value (+ records the sync baseline so a later
|
||||
@@ -669,22 +673,18 @@ export async function getGlobalDraft(
|
||||
): Promise<WorkspaceItem | undefined> {
|
||||
const itemKind = itemKindFor(type, triggerKind)
|
||||
if (!itemKind) return undefined
|
||||
return draftItemAt(workspace, itemKind, target.storagePath, target)
|
||||
return draftItemAt(workspace, itemKind, target.storagePath)
|
||||
}
|
||||
|
||||
async function draftItemAt(
|
||||
workspace: string,
|
||||
itemKind: UserDraftItemKind,
|
||||
storagePath: string,
|
||||
found: { value?: unknown; fetched?: boolean }
|
||||
storagePath: string
|
||||
): Promise<WorkspaceItem | undefined> {
|
||||
// A draft can live only as a local cell — a second session tab, or an editor with
|
||||
// autosave off — with no backend row behind it.
|
||||
const cell = UserDraft.get(itemKind, storagePath, { workspace })
|
||||
const value =
|
||||
cell ??
|
||||
found.value ??
|
||||
(found.fetched ? undefined : await fetchBackendDraftValue(workspace, itemKind, storagePath))
|
||||
const value = cell ?? (await fetchBackendDraftValue(workspace, itemKind, storagePath))
|
||||
if (value === undefined || value === null) return undefined
|
||||
const { displayPath, isLiveDraft } = liveDisplayPath(workspace, itemKind, storagePath)
|
||||
return userDraftEntryToWorkspaceItem(
|
||||
|
||||
@@ -1227,10 +1227,6 @@ export type ResolvedDraftTarget = {
|
||||
/** The path the call named. What the model and the user see, so messages quote this. */
|
||||
path: string
|
||||
storagePath: string
|
||||
/** The draft's value when resolving it already read one, so the body does not refetch. */
|
||||
value?: unknown
|
||||
/** Whether `value` reflects a read: `undefined` then means no draft, not "not looked". */
|
||||
fetched: boolean
|
||||
}
|
||||
|
||||
export interface Tool<T> {
|
||||
|
||||
Reference in New Issue
Block a user