mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-20 00:03:21 +00:00
fix(sessions): publish the removal marker only once the discard lands
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KcyBRxE8j3ujCYkum9KrC4
This commit is contained in:
co-authored by
Claude Opus 5
parent
98e30f0878
commit
c9c3b23585
@@ -2136,6 +2136,30 @@ describe('global AI tools', () => {
|
||||
).toBeUndefined()
|
||||
})
|
||||
|
||||
// The removal marker tells a session's hosted editor that its item is gone, and
|
||||
// it is read by the tool-completion listener — which a throw never reaches. Left
|
||||
// behind by a failed discard it would be consumed by some later action on the
|
||||
// same item and send that editor away while the item is still there.
|
||||
it('records the draft-only removal marker only once the discard has landed', async () => {
|
||||
const path = 'f/resources/discard-marker'
|
||||
await callGlobalTool('write_resource', {
|
||||
path,
|
||||
resource_type: 'postgresql',
|
||||
value: { host: 'localhost' }
|
||||
})
|
||||
|
||||
// The delete is a `value: null` draft write, so failing that write fails it.
|
||||
failingWrites.add(`resource:${path}`)
|
||||
await expect(callGlobalTool('discard_local_draft', { type: 'resource', path })).rejects.toThrow()
|
||||
expect(UserDraft.takeDraftOnlyDiscard('resource', path, { workspace: WORKSPACE })).toBe(false)
|
||||
|
||||
// The same discard, allowed through, does publish it (so the check above is
|
||||
// about the failure, not about the marker never being written at all).
|
||||
failingWrites.delete(`resource:${path}`)
|
||||
await callGlobalTool('discard_local_draft', { type: 'resource', path })
|
||||
expect(UserDraft.takeDraftOnlyDiscard('resource', path, { workspace: WORKSPACE })).toBe(true)
|
||||
})
|
||||
|
||||
// "Create a resource, then never mind": delete_workspace_item must reject a path
|
||||
// that was never deployed, before the confirmation card — otherwise the user
|
||||
// confirms a workspace mutation that 404s past the draft cleanup, leaving the
|
||||
|
||||
@@ -6170,17 +6170,23 @@ async function discardLocalDraft(
|
||||
throw new Error(`No draft found for ${type} "${path}".`)
|
||||
}
|
||||
|
||||
// Before the delete, while both sides are still knowable: a draft with nothing
|
||||
// deployed under it IS the item, so discarding it removes the item rather than
|
||||
// reverting it — and anything showing that item has to stop.
|
||||
// Probed before the delete, while both sides are still knowable: a draft with
|
||||
// nothing deployed under it IS the item, so discarding it removes the item
|
||||
// rather than reverting it — and anything showing that item has to stop.
|
||||
const discardedKind = itemKindFor(type, triggerKind)
|
||||
const storagePath = getGlobalDraftStoragePath(workspace, type, path, triggerKind)
|
||||
if (discardedKind && !(await hasDeployedItem(workspace, type, path))) {
|
||||
UserDraft.recordDraftOnlyDiscard(discardedKind, storagePath, { workspace })
|
||||
}
|
||||
const removesItem = !!discardedKind && !(await hasDeployedItem(workspace, type, path))
|
||||
|
||||
await deleteGlobalDraft(workspace, type, path, triggerKind)
|
||||
|
||||
// Published only now: the delete above can throw, and the marker is read by the
|
||||
// tool-completion listener, which a throw never reaches — leaving it to be
|
||||
// consumed by some later action on this item, which would send its editor away
|
||||
// while the item is still there.
|
||||
if (removesItem && discardedKind) {
|
||||
UserDraft.recordDraftOnlyDiscard(discardedKind, storagePath, { workspace })
|
||||
}
|
||||
|
||||
// The chat's touch on the item is undone — drop it from the mask so a
|
||||
// pre-existing deployed item doesn't keep reading as this chat's edit.
|
||||
if (discardedKind) {
|
||||
|
||||
@@ -2,6 +2,13 @@ import type { SessionPreviewTab } from './sessionState.svelte'
|
||||
import { whereIs } from './sessionPreviewTabs.svelte'
|
||||
import { stripBase, TRIGGER_PAGES, type TriggerKind } from './previewPaths'
|
||||
|
||||
/** What a tool asks of a hosted entity editor (see previewRouter's
|
||||
* `IN_REALM_ENTITY_PAGES`) showing one of the affected pages. A plain write
|
||||
* reaches it on its own — it holds the draft cell the write seeds — but the
|
||||
* tools that clear or replace that cell go behind it, and the item can be gone
|
||||
* altogether. `none` is what makes the live-editing case live. */
|
||||
export type EntityToolEffect = 'none' | 'refresh' | 'close'
|
||||
|
||||
// Which list pages a completed chat tool can change, as base-stripped paths
|
||||
// (e.g. `/schedules`). This allowlist is the single source of truth for "does
|
||||
// this tool change a list page a preview tab might show". A new mutating tool
|
||||
@@ -15,13 +22,6 @@ import { stripBase, TRIGGER_PAGES, type TriggerKind } from './previewPaths'
|
||||
// no list page we preview lists open drafts. They fall through to NO_RELOAD.
|
||||
// This "live editors self-sync, only list pages reload" invariant is the reason
|
||||
// the callers below and in the sessions page reload nothing for item tabs.
|
||||
/** What a tool asks of a hosted entity editor (see previewRouter's
|
||||
* `IN_REALM_ENTITY_PAGES`) showing one of the affected pages. A plain write
|
||||
* reaches it on its own — it holds the draft cell the write seeds — but the
|
||||
* tools that clear or replace that cell go behind it, and the item can be gone
|
||||
* altogether. `none` is what makes the live-editing case live. */
|
||||
export type EntityToolEffect = 'none' | 'refresh' | 'close'
|
||||
|
||||
export type ToolReloadEffect = {
|
||||
pages: string[]
|
||||
entity: EntityToolEffect
|
||||
|
||||
@@ -129,6 +129,21 @@ const entries = new Map<string, DraftEntry>()
|
||||
const seedMisses = new Set<string>()
|
||||
// Cells whose last discard removed the item itself (see `takeDraftOnlyDiscard`).
|
||||
const draftOnlyDiscards = new Set<string>()
|
||||
|
||||
// Both sets above are read by the tool-completion listener right after the write
|
||||
// that wrote them — but only while something is listening, and nothing is when the
|
||||
// user is not in a session. Capped so an unread marker cannot accumulate: the
|
||||
// oldest is dropped, since a marker is only ever meaningful to the action that
|
||||
// immediately follows it.
|
||||
const MAX_WRITE_MARKERS = 64
|
||||
function noteMarker(set: Set<string>, key: string): void {
|
||||
set.add(key)
|
||||
while (set.size > MAX_WRITE_MARKERS) {
|
||||
const oldest = set.values().next().value
|
||||
if (oldest === undefined || oldest === key) break
|
||||
set.delete(oldest)
|
||||
}
|
||||
}
|
||||
const liveEditorDrafts = new Map<string, LiveEditorDraft>()
|
||||
/**
|
||||
* Map keys whose entry should start `syncSuspended` on acquire. Lets
|
||||
@@ -409,7 +424,7 @@ export const UserDraft = {
|
||||
const mk = mapKey(ws, itemKind, path)
|
||||
const entry = entries.get(mk)
|
||||
if (!entry) {
|
||||
seedMisses.add(mk)
|
||||
noteMarker(seedMisses, mk)
|
||||
return
|
||||
}
|
||||
seedMisses.delete(mk)
|
||||
@@ -424,7 +439,7 @@ export const UserDraft = {
|
||||
* showing it. Set at the discard, where the deployed side is known.
|
||||
*/
|
||||
recordDraftOnlyDiscard(itemKind: UserDraftItemKind, path: string, opts?: UserDraftOptions): void {
|
||||
draftOnlyDiscards.add(mapKey(resolveWorkspace(opts), itemKind, path))
|
||||
noteMarker(draftOnlyDiscards, mapKey(resolveWorkspace(opts), itemKind, path))
|
||||
},
|
||||
|
||||
/** Whether the last discard for this cell removed the item outright (see
|
||||
|
||||
Reference in New Issue
Block a user