mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-16 00:03:02 +00:00
fix: follow a deployed draft's preview, and search drafts by chosen name
Review round findings: - `list_workspace_items`' `query` filter read only `path` and `summary`, so a draft listed under its storage path stopped matching the name the user searches by. - `read_workspace_item` rebuilt app and flow payloads without `draftPath`, so a read gave the model the storage path and no chosen name. - A session preview is opened on the path the draft is stored at, which the deploy then removes. The deploy notification now carries both that path and the deployed one, and the session follows the tab to the item's new path. - The name table is loaded only for a tool call that names a path, and concurrent callers share one request rather than each making their own. - The app deploy guard refused the retry of a deploy that created the app and then failed to remove its draft; `force` now passes through it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
c4b4961ff6
commit
b8a3887bbc
@@ -2446,6 +2446,53 @@ describe('global AI tools', () => {
|
||||
expect(getBackendDraft('raw_app', 'u/admin/staged_app')).toBeUndefined()
|
||||
})
|
||||
|
||||
// The chosen name is what the user searches by and what the model should call the app,
|
||||
// and it is the only place it survives once the draft is listed under its storage path.
|
||||
it('finds and names a staged app by its chosen name', async () => {
|
||||
seedBackendDraft(
|
||||
'raw_app',
|
||||
'u/admin/draft_named',
|
||||
{
|
||||
summary: 'Quote tool',
|
||||
draft_path: 'f/sales/quote_app',
|
||||
files: {},
|
||||
runnables: {},
|
||||
data: { tables: [] }
|
||||
},
|
||||
{ workspace: WORKSPACE }
|
||||
)
|
||||
|
||||
const listed = JSON.parse(
|
||||
await callGlobalTool('list_workspace_items', { types: ['app'], query: 'quote_app' })
|
||||
)
|
||||
expect(listed).toContainEqual(
|
||||
expect.objectContaining({ path: 'u/admin/draft_named', draftPath: 'f/sales/quote_app' })
|
||||
)
|
||||
const read = JSON.parse(
|
||||
await callGlobalTool('read_workspace_item', { type: 'app', path: 'f/sales/quote_app' })
|
||||
)
|
||||
expect(read).toMatchObject({ path: 'u/admin/draft_named', draftPath: 'f/sales/quote_app' })
|
||||
})
|
||||
|
||||
it('opens a preview on the draft a chosen name belongs to', async () => {
|
||||
seedBackendDraft(
|
||||
'raw_app',
|
||||
'u/admin/draft_previewed',
|
||||
{ summary: 'Previewed', draft_path: 'f/sales/preview_app', files: {}, runnables: {} },
|
||||
{ workspace: WORKSPACE }
|
||||
)
|
||||
const handler = vi.fn(async () => 'opened')
|
||||
setOpenPreviewHandler(handler)
|
||||
try {
|
||||
await callGlobalTool('open_preview', { kind: 'raw_app', path: 'f/sales/preview_app' })
|
||||
expect(handler).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ kind: 'raw_app', path: 'u/admin/draft_previewed' })
|
||||
)
|
||||
} finally {
|
||||
setOpenPreviewHandler(undefined)
|
||||
}
|
||||
})
|
||||
|
||||
it('refuses a draft_path that two drafts are staged under', async () => {
|
||||
for (const storage of ['u/admin/draft_a', 'u/admin/draft_b']) {
|
||||
seedBackendDraft(
|
||||
@@ -4739,7 +4786,8 @@ describe('global AI tools', () => {
|
||||
expect(onDeployed).toHaveBeenCalledWith({
|
||||
sessionId: 'sess-123',
|
||||
kind: 'raw_app',
|
||||
path: 'f/apps/report'
|
||||
path: 'f/apps/report',
|
||||
deployedPath: 'f/apps/report'
|
||||
})
|
||||
} finally {
|
||||
setDeployedInSessionHandler(undefined)
|
||||
|
||||
@@ -1468,13 +1468,16 @@ function getRequestedTypes(types: WorkspaceItemType[] | undefined): WorkspaceIte
|
||||
}
|
||||
|
||||
function itemMatches(
|
||||
item: Pick<WorkspaceItem, 'path' | 'summary'>,
|
||||
item: Pick<WorkspaceItem, 'path' | 'summary' | 'draftPath'>,
|
||||
query: string | undefined
|
||||
): boolean {
|
||||
const normalized = query?.trim().toLowerCase()
|
||||
if (!normalized) return true
|
||||
return (
|
||||
item.path.toLowerCase().includes(normalized) ||
|
||||
// A draft is listed under its storage path, so its chosen name — the one the user
|
||||
// searches by — is only in `draftPath`.
|
||||
(item.draftPath?.toLowerCase().includes(normalized) ?? false) ||
|
||||
(item.summary?.toLowerCase().includes(normalized) ?? false)
|
||||
)
|
||||
}
|
||||
@@ -1532,6 +1535,7 @@ function serializeWorkspaceItemForRead(item: WorkspaceItem): unknown {
|
||||
return {
|
||||
type: 'app',
|
||||
path: item.path,
|
||||
draftPath: item.draftPath,
|
||||
summary: item.summary,
|
||||
value: summarizeAppValue(item.value as AppDraftValue),
|
||||
rawApp: item.rawApp,
|
||||
@@ -1547,6 +1551,7 @@ function serializeWorkspaceItemForRead(item: WorkspaceItem): unknown {
|
||||
return {
|
||||
type: 'flow',
|
||||
path: item.path,
|
||||
draftPath: item.draftPath,
|
||||
summary: item.summary,
|
||||
value: editable,
|
||||
isDraft: item.isDraft
|
||||
@@ -4454,8 +4459,10 @@ const unroutedGlobalTools: Tool<{}>[] = [
|
||||
// Every tool reaches drafts through the adapter's resolver, which can route a draft's
|
||||
// chosen name to where it is stored only with the user's current drafts loaded.
|
||||
export const globalTools: Tool<{}>[] = unroutedGlobalTools.map((tool) => {
|
||||
// `path` is the only argument a tool addresses a draft by, so a call without one — a
|
||||
// doc search, a screenshot — needs no names and pays no request for them.
|
||||
const load = (p: { workspace: string; args: any }) =>
|
||||
loadDraftNames(p.workspace, typeof p.args?.path === 'string' ? p.args.path : undefined)
|
||||
typeof p.args?.path === 'string' ? loadDraftNames(p.workspace, p.args.path) : Promise.resolve()
|
||||
return {
|
||||
...tool,
|
||||
// Also on the pre-confirmation check: it reads drafts too, and runs before `fn`.
|
||||
@@ -4804,7 +4811,10 @@ function getSessionScreenshot(sessionId: string | undefined): Promise<SessionScr
|
||||
export type DeployedInSessionHandler = (req: {
|
||||
sessionId: string | undefined
|
||||
kind: 'script' | 'flow' | 'raw_app'
|
||||
/** Where the draft the preview is open on was stored — the deploy removes it. */
|
||||
path: string
|
||||
/** Where the item now lives, which is a different path for a draft-only or renamed item. */
|
||||
deployedPath: string
|
||||
}) => void
|
||||
|
||||
let deployedInSessionHandler: DeployedInSessionHandler | undefined
|
||||
@@ -7961,6 +7971,9 @@ async function deployDraft(
|
||||
})
|
||||
|
||||
let actions: ToolDisplayAction[] | undefined
|
||||
// Resolved before the deploy removes the draft, since both callbacks below name the
|
||||
// path the draft was addressable at, not the one it was addressed by.
|
||||
const draftStoragePath = getGlobalDraftStoragePath(workspace, type, path, triggerKind)
|
||||
// Where the deploy actually lands — the app branch can resolve a different
|
||||
// target from the draft's own path fields; the mask rename below must track it.
|
||||
let deployedPath = path
|
||||
@@ -8205,10 +8218,13 @@ 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.
|
||||
if (targetExists && targetPath !== storagePath) {
|
||||
// `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) {
|
||||
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.`
|
||||
`Ask the user for a different path for this draft, or pass force: true if this draft ` +
|
||||
`is what is deployed there.`
|
||||
)
|
||||
}
|
||||
if (targetExists) {
|
||||
@@ -8270,11 +8286,7 @@ async function deployDraft(
|
||||
// stop matching anything after the draft is gone.
|
||||
const deployedKind = itemKindFor(type, triggerKind)
|
||||
if (deployedKind) {
|
||||
toolCallbacks.onItemDeployed?.(
|
||||
deployedKind,
|
||||
getGlobalDraftStoragePath(workspace, type, path, triggerKind),
|
||||
deployedPath
|
||||
)
|
||||
toolCallbacks.onItemDeployed?.(deployedKind, draftStoragePath, deployedPath)
|
||||
}
|
||||
|
||||
// Reload the session preview if it's open on the deployed item. Map the
|
||||
@@ -8286,7 +8298,9 @@ async function deployDraft(
|
||||
app: 'raw_app'
|
||||
}
|
||||
const kind = previewKindByType[type]
|
||||
if (kind) deployedInSessionHandler?.({ sessionId, kind, path })
|
||||
if (kind) {
|
||||
deployedInSessionHandler?.({ sessionId, kind, path: draftStoragePath, deployedPath })
|
||||
}
|
||||
|
||||
toolCallbacks.setToolStatus(toolId, {
|
||||
content: `Deployed ${type} "${path}"`,
|
||||
|
||||
@@ -338,7 +338,20 @@ const deployedExists: Partial<
|
||||
* use is often not where the draft lives. Called before every chat tool call; `path` is
|
||||
* the path that call addresses, checked against deployed items only when it is a name.
|
||||
*/
|
||||
export async function loadDraftNames(workspace: string, path?: string): Promise<void> {
|
||||
export function loadDraftNames(workspace: string, path?: string): Promise<void> {
|
||||
// A tool and its own pre-confirmation check ask for the same names back to back, and
|
||||
// tools in one turn can overlap; they share the request rather than repeating it.
|
||||
const key = `${workspace}:${path ?? ''}`
|
||||
const started = loadingDraftNames.get(key)
|
||||
if (started) return started
|
||||
const loading = readDraftNames(workspace, path).finally(() => loadingDraftNames.delete(key))
|
||||
loadingDraftNames.set(key, loading)
|
||||
return loading
|
||||
}
|
||||
|
||||
const loadingDraftNames = new Map<string, Promise<void>>()
|
||||
|
||||
async function readDraftNames(workspace: string, path?: string): Promise<void> {
|
||||
const names: DraftNames = { stored: new Set(), byName: new Map(), deployedAt: new Set() }
|
||||
const add = (
|
||||
itemKind: UserDraftItemKind,
|
||||
|
||||
@@ -604,6 +604,24 @@ export class SessionPreviewTabs {
|
||||
this.#flush()
|
||||
}
|
||||
|
||||
/** Follow an editor item to the path it deployed under, in place. A draft is hosted
|
||||
* under the path it is stored at, which the deploy then removes. */
|
||||
retargetEditorItem(from: SessionTarget, to: SessionTarget): void {
|
||||
if (from.kind === to.kind && from.path === to.path) return
|
||||
const tab = this.#tabs.find((t) => isEditorTabFor(t.url, from))
|
||||
if (!tab) return
|
||||
// Two tabs on one item would mount two editors racing the same cell, as in
|
||||
// open()/navigate(); the tab already there wins and this one is closed.
|
||||
if (this.#tabs.some((t) => t.id !== tab.id && isEditorTabFor(t.url, to))) {
|
||||
this.close(tab.id)
|
||||
return
|
||||
}
|
||||
const target = previewTargetForSessionTarget(to.kind, to.path)
|
||||
if (!target) return
|
||||
retargetTab(tab, targetUrl(target))
|
||||
this.#flush()
|
||||
}
|
||||
|
||||
/** Follow a page item its editor saved under a new path, in place. */
|
||||
retargetPageItem(from: PageItemRef, to: PageItemRef): void {
|
||||
const fromUrl = pageItemUrl(from)
|
||||
|
||||
@@ -1273,7 +1273,7 @@ setClosePreviewTabsHandler(({ sessionId: callerSessionId, all, match }) => {
|
||||
|
||||
// After a chat deploy, reload the calling session's preview — only if it's open
|
||||
// showing that exact item.
|
||||
setDeployedInSessionHandler(({ sessionId: callerSessionId, kind, path }) => {
|
||||
setDeployedInSessionHandler(({ sessionId: callerSessionId, kind, path, deployedPath }) => {
|
||||
const sessionId = callerSessionId ?? sessionState.currentSessionId
|
||||
if (!sessionId) return
|
||||
const session = sessionState.sessions.find((s) => s.id === sessionId)
|
||||
@@ -1282,7 +1282,14 @@ setDeployedInSessionHandler(({ sessionId: callerSessionId, kind, path }) => {
|
||||
// Peek without creating a cell: a deploy for an item with no open editor tab
|
||||
// must not allocate an empty cell that lingers until the next prune.
|
||||
if (runtime.loadedEditorPath(kind, path) !== path) return
|
||||
runtime.syncPreviewWithDeployed(session.workspace_id, kind, path)
|
||||
if (deployedPath !== path) {
|
||||
// The draft the tab is open on is gone: drop it and follow the item to the path
|
||||
// it now lives at, so the tab doesn't sit on a path nothing resolves to.
|
||||
UserDraft.stopSync(kind, path, { workspace: session.workspace_id })
|
||||
UserDraft.discard(kind, path, undefined, { workspace: session.workspace_id })
|
||||
runtime.previewTabs.retargetEditorItem({ kind, path }, { kind, path: deployedPath })
|
||||
}
|
||||
runtime.syncPreviewWithDeployed(session.workspace_id, kind, deployedPath)
|
||||
})
|
||||
|
||||
setGetRuntimeLogsHandler(async ({ sessionId: callerSessionId, limit }) => {
|
||||
|
||||
Reference in New Issue
Block a user