diff --git a/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte b/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte index 0ab2812b2e..a181cdebd3 100644 --- a/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte +++ b/frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte @@ -235,6 +235,7 @@ switch that decides whether this chat carries its tools. // A failed save leaves the connection exactly as it was, so none of the // bookkeeping below may run: moving the enablement then would turn a server // that still exists off, and turn on a path that was never created. + // False whenever a write threw, a partial multi-workspace save included. if (!(await resourceEditor?.save())) return // Enablement is keyed by path, so a rename would leave the switch on the path // that no longer exists and the server itself off. diff --git a/frontend/src/lib/components/copilot/chat/global/core.test.ts b/frontend/src/lib/components/copilot/chat/global/core.test.ts index 90ff8a8f77..7a5546f478 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -2158,6 +2158,107 @@ 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) + }) + + // The listener that refreshes hosted editors runs only for a tool that returned, + // so a throw after the entity is gone leaves a tab open on an item that no longer + // exists — and the cleanup cannot un-delete it either way. + it('reports a delete whose draft cleanup failed instead of throwing', async () => { + const path = 'f/resources/deleted-with-stuck-draft' + await callGlobalTool('write_resource', { + path, + resource_type: 'postgresql', + value: { host: 'localhost' } + }) + // The cleanup is a `value: null` draft write, so failing that write fails it. + failingWrites.add(`resource:${path}`) + + const result = await callGlobalTool('delete_workspace_item', { type: 'resource', path }) + + expect(ResourceService.deleteResource).toHaveBeenCalled() + expect(JSON.parse(result).message).toMatch(/draft could NOT be cleared/) + }) + + // Nothing consumes a marker outside a session, so one can outlive the item it + // was about. Recreating the item must void it, or the next action on the + // recreated item reads it and sends a perfectly valid editor back to its list. + it('voids a draft-only removal marker when the item is written again', async () => { + const path = 'f/resources/discard-then-recreate' + await callGlobalTool('write_resource', { + path, + resource_type: 'postgresql', + value: { host: 'localhost' } + }) + await callGlobalTool('discard_local_draft', { type: 'resource', path }) + + // The marker is standing (no listener consumed it), and now the item is back. + await callGlobalTool('write_resource', { + path, + resource_type: 'postgresql', + value: { host: 'elsewhere' } + }) + expect(UserDraft.takeDraftOnlyDiscard('resource', path, { workspace: WORKSPACE })).toBe(false) + }) + + // A marker nothing read outlives the item it was about: the path can be deployed + // again from another tab or client, and the next discard there — a revert, not a + // removal — would otherwise spend it and send that item's editor away. + describe('a removal marker the deployed item outlived', () => { + // The suite's beforeEach clears calls but not implementations, so the + // `existsResource` override below goes back to its factory default here rather + // than inline, where a failing assertion would leak it into every later test. + afterEach(() => { + vi.mocked(ResourceService.existsResource).mockResolvedValue(false) + }) + + it('is voided when the path is deployed again', async () => { + const path = 'f/resources/discard-then-deployed' + await callGlobalTool('write_resource', { + path, + resource_type: 'postgresql', + value: { host: 'localhost' } + }) + await callGlobalTool('discard_local_draft', { type: 'resource', path }) + + // Deployed since from another client, with a draft of its own — neither reaches + // this tab's cell, so nothing has cleared the marker. Discarding that draft + // reverts the item rather than removing it. + vi.mocked(ResourceService.existsResource).mockResolvedValue(true) + seedBackendDraft('resource', path, { + path, + value: { host: 'elsewhere' }, + resource_type: 'postgresql' + }) + await callGlobalTool('discard_local_draft', { type: 'resource', path }) + + expect(UserDraft.takeDraftOnlyDiscard('resource', path, { workspace: WORKSPACE })).toBe(false) + }) + }) + // "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 diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index 5883c5ae0d..9fa2678f72 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -6589,6 +6589,25 @@ function createOpenVariableAction(path: string): ToolDisplayAction { } } +/** Whether a deployed item exists under a draft, for the kinds whose editor a + * session hosts. Anything else answers true — the caller only uses this to decide + * whether discarding leaves nothing behind, and no other kind is hosted. */ +async function hasDeployedItem( + workspace: string, + type: WorkspaceItemType, + path: string +): Promise { + try { + if (type === 'resource') return await ResourceService.existsResource({ workspace, path }) + if (type === 'variable') return await VariableService.existsVariable({ workspace, path }) + if (type === 'schedule') return await ScheduleService.existsSchedule({ workspace, path }) + } catch { + // The probe is an optimisation over "assume it survived"; a failed one must + // not fail the discard, and treating it as deployed keeps today's behavior. + } + return true +} + async function discardLocalDraft( args: { type: WorkspaceItemType; path: string; trigger_kind?: TriggerKind }, ctx: WriteDraftCtx @@ -6605,16 +6624,40 @@ async function discardLocalDraft( throw new Error(`No draft found for ${type} "${path}".`) } + // 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) + const removesItem = !!discardedKind && !(await hasDeployedItem(workspace, type, path)) + await deleteGlobalDraft(workspace, type, path, triggerKind) + // Published only now, and only if nothing is queued behind the delete: the marker + // sends a hosted editor away from an item it says is gone, so a throw above must + // not leave one standing for a later action to spend, and an edit made across the + // delete recreates the draft — leaving the item the marker would report as gone. + if (removesItem && discardedKind) { + const query = { workspace, itemKind: discardedKind, path: storagePath } + // Settled AND settled on the delete: an upsert queued behind it displaces the + // delete, and waiting for the chain then reports idle on a draft that is back. + const removed = + UserDraftDbSyncer.getState(query).state === 'none' && + UserDraftDbSyncer.lastLandedWasDelete(query) + if (removed) { + UserDraft.recordDraftOnlyDiscard(discardedKind, storagePath, { workspace }) + } + } else if (discardedKind) { + // The item is deployed, so this discard reverted it rather than removing it — + // which also settles any marker still standing from an earlier one at this + // path, recorded when nothing deployed was there and never read. + UserDraft.clearDraftOnlyDiscard(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. - const discardedKind = itemKindFor(type, triggerKind) if (discardedKind) { - toolCallbacks.onItemDiscarded?.( - discardedKind, - getGlobalDraftStoragePath(workspace, type, path, triggerKind) - ) + toolCallbacks.onItemDiscarded?.(discardedKind, storagePath) } toolCallbacks.setToolStatus(toolId, { @@ -7989,12 +8032,23 @@ async function deployDraft( } } + // Where the drawer kinds land is the draft's own path field, not the key it is + // stored under: a hosted editor renames a draft-only item by editing the config, + // which leaves the draft where it was. The script/flow/app branches resolve their + // own target above. + if (type !== 'script' && type !== 'flow' && type !== 'app') { + const draftPath = (draft.value as { path?: string } | undefined)?.path + if (draftPath) deployedPath = draftPath + } + // Deployed state moved for EVERY branch above (some bypass // deployDraftToWorkspace, which invalidates on its own path) — evict cached // fork comparisons before the fallible draft cleanup below. invalidateWorkspaceComparison(workspace) - await deleteGlobalDraft(workspace, type, path, triggerKind, { preserveLiveDraft: true }) + const draftIssue = await clearDraftAfterMutation(workspace, type, path, triggerKind, { + preserveLiveDraft: true + }) // Move the chat's mask entry to the deployed path: a draft-only item's // synthetic storage key never exists deployed, so the entry would otherwise @@ -8027,11 +8081,19 @@ async function deployDraft( return JSON.stringify( { success: true, - message: `Deployed draft ${type} "${path}" to the workspace. Draft removed.${ - deployNote ? ` ${deployNote}` : '' - }`, + message: [ + `Deployed draft ${type} "${path}" to the workspace.`, + draftIssue + ? `The draft could NOT be removed (${draftIssue}), so the item may still show unsaved changes.` + : 'Draft removed.', + deployNote + ] + .filter(Boolean) + .join(' '), type, path, + // Where it landed, which a rename staged in the draft moves off `path`. + deployed_path: deployedPath, triggerKind }, null, @@ -8132,6 +8194,23 @@ async function deployedItemExists( } } +/** + * Clear the draft of an item whose deployed state has already changed. A failure + * here cannot undo that change, and throwing would report the mutation as not + * having happened — to the model, and to the hosts, which hear about one only + * from a tool that returned. Reported in the result instead. + */ +async function clearDraftAfterMutation( + ...args: Parameters +): Promise { + try { + await deleteGlobalDraft(...args) + return undefined + } catch (e) { + return e instanceof Error ? e.message : String(e) + } +} + async function deleteWorkspaceItem( args: { type: WorkspaceItemType; path: string; trigger_kind?: TriggerKind }, ctx: WriteDraftCtx @@ -8175,7 +8254,7 @@ async function deleteWorkspaceItem( // are no longer trustworthy (same rule as deploy success). Before the // draft cleanup: a cleanup failure must not leave stale comparisons. invalidateWorkspaceComparison(workspace) - await deleteGlobalDraft(workspace, type, path, triggerKind) + const draftIssue = await clearDraftAfterMutation(workspace, type, path, triggerKind) // Record the deletion in the chat's modified-items mask. In a fork this leaves a // reviewable "removed" diff vs the parent that stays scoped to this chat. Keyed @@ -8195,7 +8274,11 @@ async function deleteWorkspaceItem( return JSON.stringify( { success: true, - message: `Deleted ${type} "${path}" from the workspace. Any matching draft was also cleared.`, + message: + `Deleted ${type} "${path}" from the workspace. ` + + (draftIssue + ? `Its draft could NOT be cleared (${draftIssue}), so the path may still list one.` + : 'Any matching draft was also cleared.'), type, path, triggerKind diff --git a/frontend/src/lib/components/copilot/chat/shared.ts b/frontend/src/lib/components/copilot/chat/shared.ts index bc00e415f6..ef51ea5fec 100644 --- a/frontend/src/lib/components/copilot/chat/shared.ts +++ b/frontend/src/lib/components/copilot/chat/shared.ts @@ -765,10 +765,12 @@ export function pendingUserActionDetail( // the sessions page) react to mutating tools — refreshing previews — without // the tool layer knowing about the UI. Single slot; the consumer filters by name // and reads the tool args (e.g. the mutated item's `path`) to scope its refresh. -let toolCompletionListener: ((toolName: string, args: any) => void) | undefined +let toolCompletionListener: + | ((toolName: string, args: any, workspace: string, result: string) => void) + | undefined export function setToolCompletionListener( - fn: ((toolName: string, args: any) => void) | undefined + fn: ((toolName: string, args: any, workspace: string, result: string) => void) | undefined ): void { toolCompletionListener = fn } @@ -797,7 +799,11 @@ async function callTool({ ) } const result = await tool.fn({ args, workspace, helpers, toolCallbacks, toolId }) - toolCompletionListener?.(functionName, args) + // The workspace the tool acted on, so a consumer can tell a mutation in this + // session's workspace from the same path in another one — and the result, which + // is where a tool says what it did that its args do not, such as the path a + // deploy landed on after a rename staged in the draft. + toolCompletionListener?.(functionName, args, workspace, result) return result } diff --git a/frontend/src/lib/components/sessions/PreviewTabHost.svelte b/frontend/src/lib/components/sessions/PreviewTabHost.svelte index 3dee7dd1b3..da0b91e0be 100644 --- a/frontend/src/lib/components/sessions/PreviewTabHost.svelte +++ b/frontend/src/lib/components/sessions/PreviewTabHost.svelte @@ -12,12 +12,17 @@ import type { SessionRuntime } from './sessionRuntime.svelte' import { Loader2 } from 'lucide-svelte' import { + entityListHref, + entityListPage, resolvePreviewTab, parsePreviewItemRoute, parsePreviewSelectedId, showsView } from './previewRouter' + import type { EntityEditorKind } from './previewRouter' + import type { EntityToolEffect } from './previewReload' import { withMenuHidden } from './sessionMode.svelte' + import { pageHref, RUNS_PATH } from './previewPaths' import ArtifactViewer from '../copilot/chat/artifacts/ArtifactViewer.svelte' import RunFormPreviewSlot from './RunFormPreviewSlot.svelte' import { setOverlayHost } from '../common/overlayHost.svelte' @@ -33,7 +38,8 @@ darkMode, fullscreen = false, onNavigate, - onLoad + onLoad, + onEntityWritten }: { tab: SessionPreviewTab session: Session | undefined @@ -59,6 +65,21 @@ onNavigate: (item: WorkspaceItem) => void /** Iframe finished loading — the page reads back its observed location. */ onLoad: (frame: HTMLIFrameElement) => void + /** A hosted entity editor wrote its item: saved it (`to`, the path it wrote + * to) or removed it. The page reaches every tab on that item, across warm + * sessions — `fromSessionId`/`fromTabId` being the one that reported, which has + * settled its own state already. Both: tab ids are unique within a session but + * the seeded first tab is `session` in all of them. */ + onEntityWritten: (ev: { + kind: EntityEditorKind + path: string + workspace: string + to?: string + fromSessionId: string | undefined + fromTabId: string + /** Whether the editor that reported is still the one mounted on this tab. */ + fromLive?: boolean + }) => void } = $props() // Editor vs iframe is decided purely from the tab URL (see resolvePreviewTab): @@ -83,6 +104,11 @@ let frame: HTMLIFrameElement | undefined = $state() + // A hosted entity editor builds its state at mount, from the draft cell and the + // workspace it was given, and can refresh neither in place — so both the bump + // below and a change of `workspaceId` key it, and remounting is how it re-reads. + let entityNonce = $state(0) + // Pages whose theme we mirror on live toggles. Regular apps are the only item // route that resolves to an iframe (scripts/flows/raw apps mount live editors) // and they pin their own theme, so excluding item routes excludes exactly them. @@ -103,12 +129,21 @@ applyPageIframeTheme(darkMode) }) - export function reload() { + export function reload(opts?: { entity?: EntityToolEffect }) { // A live editor shares the runtime store the chat mutates, so generic chat // edits are already reflected — no reload needed. Deploys refresh it via // each editor view's onDeploy → runtime.syncPreviewWithDeployed. So only the // iframe fallback (a separate page) has to be told to refresh. if (slot.kind === 'editor') return + // An entity editor holds a live UserDraft handle on the cell the chat's + // write seeds, so a write needs nothing from here — but the tools that drop + // that cell go behind it, and it has to be re-read from the server. + // Deletion is not handled here: it belongs to the tab whose item is gone, + // mounted or not, so the page re-points that tab by id instead. + if (slot.kind === 'entity') { + if (opts?.entity === 'refresh') entityNonce++ + return + } try { const win = frame?.contentWindow if (!win) return @@ -141,6 +176,79 @@ active ? 'z-10 opacity-100 pointer-events-auto' : 'z-0 opacity-0 pointer-events-none' ) + // An entity editor replaced the list its tab was opened from (the row is the + // tab now, not a drawer over the list), so it has to offer the way back. + // Re-points this tab rather than opening another: the list is where the tab + // came from, not a second destination. + const backToList = $derived( + slot.kind === 'entity' && runtime + ? () => { + const page = entityListPage(slot.entityKind) + if (page) + runtime.previewTabs.navigate({ + // The tab's own location, not the page's bare path: it carries the + // query of the list the row was opened from, and returning to an + // unfiltered list is not returning to where the tab came from. + type: 'page', + href: entityListHref(whereIs(tab)), + label: page.label + }) + } + : undefined + ) + + // "View runs" from a hosted editor: the Runs page is a preview page like any + // other, so it belongs in the preview rather than in the top-level document the + // anchor would navigate — and the frame gives it the acting workspace. Re-points + // this tab, like the way back to the list: the runs are where the editor was. + const viewRuns = $derived( + runtime + ? (query: string) => + runtime.previewTabs.navigate({ + type: 'page', + href: `${pageHref(RUNS_PATH)}?${query}`, + label: 'Runs' + }) + : undefined + ) + + // What an editor reports is about an item, not about this tab: other warm + // sessions can hold the same one, and by the time the write it awaited returns + // this tab may hold something else. The page resolves both by matching on the + // item, so a report gone stale simply finds nothing. + const reportRemoved = $derived( + slot.kind === 'entity' + ? (kind: EntityEditorKind, fromPath: string, fromWs: string) => + onEntityWritten({ + kind, + path: fromPath, + workspace: fromWs, + fromSessionId: session?.id, + fromTabId: tab.id + }) + : undefined + ) + const reportSaved = $derived( + slot.kind === 'entity' + ? ( + kind: EntityEditorKind, + newPath: string, + fromPath: string, + fromWs: string, + fromLive: boolean + ) => + onEntityWritten({ + kind, + path: fromPath, + workspace: fromWs, + to: newPath, + fromSessionId: session?.id, + fromTabId: tab.id, + fromLive + }) + : undefined + ) + // Overlays a tab opens (drawers, modals, popovers) anchor here rather than to the // document, so they stay within this tab and hide with it when another tab takes over. // Every branch that renders content in-realm must bind this — an unbound host makes the @@ -311,6 +419,60 @@ {/await} {/if} +{:else if slot.kind === 'entity' && mounted} +
+ + + {#key `${workspaceId}#${entityNonce}#${slot.path}`} + {#if slot.entityKind === 'trigger_schedule'} + {#await import('./ScheduleEditorView.svelte')} + {@render editorLoading()} + {:then Module} + reportRemoved?.('trigger_schedule', from, ws)} + onSavedTo={(to, from, ws, live) => + reportSaved?.('trigger_schedule', to, from, ws, live)} + onViewRuns={viewRuns} + /> + {/await} + {:else if slot.entityKind === 'resource'} + {#await import('./ResourceEditorView.svelte')} + {@render editorLoading()} + {:then Module} + reportRemoved?.('resource', from, ws)} + onSavedTo={(to, from, ws, live) => reportSaved?.('resource', to, from, ws, live)} + /> + {/await} + {:else if slot.entityKind === 'variable'} + {#await import('./VariableEditorView.svelte')} + {@render editorLoading()} + {:then Module} + reportRemoved?.('variable', from, ws)} + onSavedTo={(to, from, ws, live) => reportSaved?.('variable', to, from, ws, live)} + /> + {/await} + {/if} + {/key} +
{:else if slot.kind === 'artifact' && mounted}
+ import ResourceEditorDrawer from '$lib/components/ResourceEditorDrawer.svelte' + import { untrack, onDestroy } from 'svelte' + + let { + path, + workspaceId, + onBack, + onRemoved, + onSavedTo + }: { + /** The resource this tab edits (the row its location deep-links). */ + path: string + /** The session's acting workspace, which the editor operates on instead of + * `$workspaceStore` (the nav workspace, which a session leaves put). */ + workspaceId: string + /** Back to the list this editor was reached through; the tab replaced it. */ + onBack?: () => void + /** The item at `fromPath` is gone — a draft-only one whose draft was discarded. + * Distinct from `onBack`, which moves whichever tab is active: only this tab is + * the one to send back, and only while it still shows that item. */ + onRemoved?: (fromPath: string, fromWorkspace: string) => void + /** The item at `fromPath` was saved, to `newPath`. Every tab on it has to hear: + * a rename moves the ones addressing it by path — their label, the chat's + * ACTIVE PREVIEW and the draft key all do — and a plain save moves none but + * leaves the others holding a baseline the deploy has replaced. */ + onSavedTo?: ( + newPath: string, + fromPath: string, + fromWorkspace: string, + /** Whether the editor that saved is still the one mounted here. A tab left + * and returned to the same item during the write has another in its place, + * loaded before the save landed, which cannot have settled its own baseline. */ + fromLive: boolean + ) => void + } = $props() + + // A tab left and reopened on the same item mounts a new view over this one; the + // editor that saved is then gone, and what it reports about its baseline is not + // about the form on screen. + let mounted = true + onDestroy(() => (mounted = false)) + + let editor = $state() + + // Re-selects when the tab is pointed at another resource; the component keeps + // its identity across that, as it does for the drawer's row-to-row switch. + $effect(() => { + const p = path + const e = editor + if (!p || !e) return + untrack(() => void e.initEdit(p)) + }) + + + onSavedTo?.(restored, restored, ws, mounted)} + onSaved={(saved, from, fromWs) => { + if (saved && from && fromWs) onSavedTo?.(saved, from, fromWs, mounted) + }} +/> diff --git a/frontend/src/lib/components/sessions/ScheduleEditorView.svelte b/frontend/src/lib/components/sessions/ScheduleEditorView.svelte new file mode 100644 index 0000000000..1477ff30f1 --- /dev/null +++ b/frontend/src/lib/components/sessions/ScheduleEditorView.svelte @@ -0,0 +1,115 @@ + + +
+ + + {#key `${path}#${generation}`} + + { + if (from === path && !keptEdit) generation++ + // A draft-only schedule opens with its path editable (saving CREATEs), so + // the save can land somewhere other than where the tab is pointed. + if (saved) onSavedTo?.(saved, from, fromWs, mounted) + }} + > + {#snippet customLabel()} +
+ {#if onBack} +
+ {/snippet} + + {/key} +
diff --git a/frontend/src/lib/components/sessions/VariableEditorView.svelte b/frontend/src/lib/components/sessions/VariableEditorView.svelte new file mode 100644 index 0000000000..c450c9e67e --- /dev/null +++ b/frontend/src/lib/components/sessions/VariableEditorView.svelte @@ -0,0 +1,70 @@ + + + +{#key path} + { + if (saved && from && fromWs) onSavedTo?.(saved, from, fromWs, mounted) + }} + /> +{/key} diff --git a/frontend/src/lib/components/sessions/pageDrawerSession.ts b/frontend/src/lib/components/sessions/pageDrawerSession.ts index ca7379be20..0a3e7435ca 100644 --- a/frontend/src/lib/components/sessions/pageDrawerSession.ts +++ b/frontend/src/lib/components/sessions/pageDrawerSession.ts @@ -11,6 +11,7 @@ import type { UserDraftItemKind } from '$lib/gen' // flow editors, where pulling the filter schemas that module reads views from would make // every trigger's save utils eager. import { + drawerHashFor, pageHref, stripBase, TRIGGER_PAGES, @@ -63,11 +64,6 @@ async function flushOrRefuse(query: Parameters[0 } } -// How each page addresses a row in its hash. Resources route theirs through an extra -// segment; every other page names the path directly. -const drawerHashFor = (pagePath: string, itemPath: string) => - pagePath === RESOURCES_PATH ? `/resource/${itemPath}` : itemPath - /** * Deep-link the row whose drawer just opened, so the location says what is on screen — a * drawer opened from a row's Edit button is as open as one reached by link, and the chat diff --git a/frontend/src/lib/components/sessions/previewPaths.ts b/frontend/src/lib/components/sessions/previewPaths.ts index 8d10dc1a32..cb35a3e444 100644 --- a/frontend/src/lib/components/sessions/previewPaths.ts +++ b/frontend/src/lib/components/sessions/previewPaths.ts @@ -65,6 +65,13 @@ export function stripBase(path: string): string { return p || '/' } +/** How a list page addresses one of its rows in the hash. Resources route theirs + * through an extra segment; every other page names the path directly. The inverse + * of `drawerAnchorFor`. */ +export function drawerHashFor(pagePath: string, itemPath: string): string { + return pagePath === RESOURCES_PATH ? `/resource/${itemPath}` : itemPath +} + export type PreviewItemRoute = { kind: WorkspaceItemKind; raw_app: boolean; itemPath: string } // Parse a preview URL/pathname into the workspace item it edits, or null for a diff --git a/frontend/src/lib/components/sessions/previewReload.test.ts b/frontend/src/lib/components/sessions/previewReload.test.ts index de11805b9a..102b073a5e 100644 --- a/frontend/src/lib/components/sessions/previewReload.test.ts +++ b/frontend/src/lib/components/sessions/previewReload.test.ts @@ -1,5 +1,12 @@ import { describe, it, expect } from 'vitest' -import { toolReloadEffect, tabsToReload } from './previewReload' +import { + toolReloadEffect, + tabsToReload, + strongerEntityEffect, + entityEffectForTab, + effectForWrite, + effectForDiscard +} from './previewReload' import type { SessionPreviewTab } from './sessionState.svelte' describe('toolReloadEffect', () => { @@ -10,6 +17,25 @@ describe('toolReloadEffect', () => { expect(toolReloadEffect('create_folder', { name: 'f' }).pages).toEqual(['/folders']) }) + // A draft carries its own path field, so a hosted editor renames a draft-only + // item by editing the config and leaves the draft under the key it was stored + // at. The deploy lands on the config's path, and only the tool's own result + // says so — refreshing the args path would remount a tab on an empty one. + it('follows a deploy that landed on a renamed path', () => { + const effect = toolReloadEffect( + 'deploy_workspace_item', + { type: 'schedule', path: 'u/me/old' }, + JSON.stringify({ success: true, path: 'u/me/old', deployed_path: 'u/me/new' }) + ) + expect(effect).toMatchObject({ path: 'u/me/old', to: 'u/me/new', entity: 'refresh' }) + }) + + it('names no destination for an unreadable or absent result', () => { + const args = { type: 'schedule', path: 'u/me/s' } + expect(toolReloadEffect('deploy_workspace_item', args, 'not json').to).toBeUndefined() + expect(toolReloadEffect('deploy_workspace_item', args).to).toBeUndefined() + }) + it('maps a trigger write to its kind-specific page', () => { expect(toolReloadEffect('write_trigger', { kind: 'kafka' }).pages).toEqual(['/kafka_triggers']) expect(toolReloadEffect('write_trigger', { kind: 'http' }).pages).toEqual(['/routes']) @@ -27,6 +53,34 @@ describe('toolReloadEffect', () => { ).toEqual(['/nats_triggers']) }) + // A hosted entity editor holds the draft cell a write seeds, so a write must + // leave it alone — that is what makes the chat's edit show up live. The tools + // that drop the cell have to reach it, and a delete leaves it with no item. + it('asks a hosted entity editor to hold, refresh, or close, per tool', () => { + expect(toolReloadEffect('write_resource', {}).entity).toBe('none') + expect(toolReloadEffect('write_schedule', { path: 'u/me/s' }).entity).toBe('none') + expect(toolReloadEffect('write_trigger', { kind: 'kafka' }).entity).toBe('none') + expect(toolReloadEffect('deploy_workspace_item', { type: 'resource' }).entity).toBe('refresh') + expect(toolReloadEffect('discard_local_draft', { type: 'variable' }).entity).toBe('refresh') + expect(toolReloadEffect('rebase_draft', { type: 'schedule' }).entity).toBe('refresh') + expect(toolReloadEffect('delete_workspace_item', { type: 'resource' }).entity).toBe('close') + }) + + it('carries the mutated item path, so an editor on another item is left alone', () => { + expect( + toolReloadEffect('delete_workspace_item', { type: 'resource', path: 'u/me/a' }).path + ).toBe('u/me/a') + // No path in the args: nothing to scope by, so it reaches every editor on the page. + expect(toolReloadEffect('deploy_workspace_item', { type: 'resource' }).path).toBeUndefined() + }) + + it('takes the strongest effect across a debounced round', () => { + expect(strongerEntityEffect('none', 'refresh')).toBe('refresh') + expect(strongerEntityEffect('refresh', 'close')).toBe('close') + expect(strongerEntityEffect('close', 'refresh')).toBe('close') + expect(strongerEntityEffect('none', 'none')).toBe('none') + }) + it('reloads no page for item-editor kinds (they self-sync via their live editor)', () => { for (const type of ['script', 'flow', 'app']) { expect(toolReloadEffect('deploy_workspace_item', { type }).pages).toEqual([]) @@ -52,6 +106,77 @@ describe('toolReloadEffect', () => { }) }) +describe('effectForWrite', () => { + // A write is normally invisible to this layer — the editor holds the cell it + // seeds. The exception is an editor whose first load is still in flight: it + // holds no cell yet, `seed` no-ops, and only a re-read reconciles it. + it('asks for a refresh only when the seed found no live editor', () => { + expect(effectForWrite('none', true)).toBe('none') + expect(effectForWrite('none', false)).toBe('refresh') + }) + + it('never weakens what the tool already asked for', () => { + expect(effectForWrite('close', false)).toBe('close') + expect(effectForWrite('refresh', true)).toBe('refresh') + }) +}) + +describe('effectForDiscard', () => { + // Discarding reverts an item to what is deployed — unless nothing is, in which + // case the draft WAS the item and the editor is left showing something that no + // longer exists, remounting into a load that cannot resolve. + it('closes a hosted editor whose item the discard removed outright', () => { + expect(effectForDiscard('refresh', false)).toBe('close') + }) + + it('keeps a deployed item on screen, reverted rather than closed', () => { + expect(effectForDiscard('refresh', true)).toBe('refresh') + }) + + it('leaves the other effects alone', () => { + expect(effectForDiscard('none', false)).toBe('none') + expect(effectForDiscard('close', true)).toBe('close') + }) +}) + +describe('entityEffectForTab', () => { + const del = (path: string, workspace = 'ws1') => ({ + pages: ['/resources'], + effect: 'close' as const, + path, + workspace + }) + const tab = { listPage: '/resources', path: 'u/me/a', workspace: 'ws1' } + + it('applies a mutation to the editor on that item', () => { + expect(entityEffectForTab([del('u/me/a')], tab)).toBe('close') + }) + + // The bug this guards: deleting one resource must not shut every open resource + // editor, nor the same path in a session acting on another workspace. + it('leaves editors on another item, page, or workspace alone', () => { + expect(entityEffectForTab([del('u/me/b')], tab)).toBe('none') + expect(entityEffectForTab([del('u/me/a', 'ws2')], tab)).toBe('none') + expect(entityEffectForTab([{ ...del('u/me/a'), pages: ['/variables'] }], tab)).toBe('none') + }) + + it('reaches every editor on the page when the tool named no item', () => { + expect(entityEffectForTab([{ ...del('u/me/a'), path: undefined }], tab)).toBe('close') + }) + + it('takes the strongest of the mutations that reach it, not of the whole round', () => { + const refreshOther = { + pages: ['/resources'], + effect: 'refresh' as const, + path: 'u/me/b', + workspace: 'ws1' + } + const refreshMine = { ...refreshOther, path: 'u/me/a' } + expect(entityEffectForTab([refreshMine, del('u/me/b')], tab)).toBe('refresh') + expect(entityEffectForTab([refreshMine, del('u/me/a')], tab)).toBe('close') + }) +}) + describe('tabsToReload', () => { const scheduleTab: SessionPreviewTab = { id: 's', url: '/schedules', loc: '/schedules' } const resourceTab: SessionPreviewTab = { id: 'r', url: '/resources', loc: '/resources' } diff --git a/frontend/src/lib/components/sessions/previewReload.ts b/frontend/src/lib/components/sessions/previewReload.ts index 6e9d750a1f..3dddb2333e 100644 --- a/frontend/src/lib/components/sessions/previewReload.ts +++ b/frontend/src/lib/components/sessions/previewReload.ts @@ -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,34 +22,136 @@ 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. -export type ToolReloadEffect = { pages: string[] } -const NO_RELOAD: ToolReloadEffect = { pages: [] } +export type ToolReloadEffect = { + pages: string[] + entity: EntityToolEffect + /** The item the tool mutated, when its args name one. A hosted editor on + * another path is not affected by it — unlike a list page, which shows every + * row and so reloads for any mutation on it. */ + path?: string + /** Where it landed, when that is not `path`: a draft carries its own path + * field, so a hosted editor renames a draft-only item by editing the config + * and leaves the draft under the key it was stored at. A tab on `path` has to + * follow, exactly as it does for a rename the editor itself saved. */ + to?: string +} +const NO_RELOAD: ToolReloadEffect = { pages: [], entity: 'none' } -export function toolReloadEffect(name: string, args: any): ToolReloadEffect { +export function toolReloadEffect(name: string, args: any, result?: string): ToolReloadEffect { switch (name) { + // `path` rides along on the writes too: it is what scopes the refresh a write + // needs when its seed missed a still-loading editor (see effectForWrite). case 'write_schedule': - return { pages: ['/schedules'] } + return { pages: ['/schedules'], entity: 'none', path: itemPath(args) } case 'write_trigger': - return { pages: triggerPages(args?.kind) } + return { pages: triggerPages(args?.kind), entity: 'none', path: itemPath(args) } case 'write_resource': - return { pages: ['/resources'] } + return { pages: ['/resources'], entity: 'none', path: itemPath(args) } case 'write_variable': - return { pages: ['/variables'] } + return { pages: ['/variables'], entity: 'none', path: itemPath(args) } case 'create_folder': - return { pages: ['/folders'] } + return { pages: ['/folders'], entity: 'none' } // Generic item tools carry a workspace-item `type`; refresh its list page // when it lives on one (schedule/resource/variable/trigger). script/flow/app // have their own live editor tab and no previewed list page → nothing. - case 'delete_workspace_item': - case 'discard_local_draft': + // These all drop the draft the hosted editor is bound to: deploying or + // discarding replaces it with the deployed value, and deleting removes the + // item, so the editor is re-read from the server or left behind entirely. case 'deploy_workspace_item': + return { + pages: pagesForItemType(args?.type, args), + entity: 'refresh', + path: itemPath(args), + to: deployedPath(result) + } + case 'discard_local_draft': case 'rebase_draft': - return { pages: pagesForItemType(args?.type, args) } + return { pages: pagesForItemType(args?.type, args), entity: 'refresh', path: itemPath(args) } + case 'delete_workspace_item': + return { pages: pagesForItemType(args?.type, args), entity: 'close', path: itemPath(args) } default: return NO_RELOAD } } +/** The path a deploy reported landing on. Read from the result rather than the + * args because only the tool knows it: the draft's own path field is what it + * deploys at. Anything unparseable leaves the mutation addressed by its args. */ +function deployedPath(result: string | undefined): string | undefined { + if (!result) return undefined + try { + const p = (JSON.parse(result) as { deployed_path?: unknown }).deployed_path + return typeof p === 'string' && p ? p : undefined + } catch { + return undefined + } +} + +function itemPath(args: any): string | undefined { + // write_trigger carries the item under `config` (see writeTriggerDraft), unlike + // every other tool here, which names it at the top level. + const p = args?.path ?? args?.config?.path + return typeof p === 'string' && p ? p : undefined +} + +/** One item mutation from a chat round, as the preview needs to read it back. */ +export type EntityMutation = { + pages: string[] + effect: EntityToolEffect + path?: string + /** The workspace the tool acted on — a session on a fork must not be moved by + * a mutation to the same path in its parent. */ + workspace: string +} + +/** What a hosted entity editor showing `tab` must do about a round's mutations. + * A mutation reaches it only in its own workspace, on its own list page, and — + * when the tool named one — on its own path. */ +export function entityEffectForTab( + mutations: readonly EntityMutation[], + tab: { listPage: string; path: string; workspace: string } +): EntityToolEffect { + let effect: EntityToolEffect = 'none' + for (const m of mutations) { + if (m.workspace !== tab.workspace) continue + if (!m.pages.includes(tab.listPage)) continue + if (m.path !== undefined && m.path !== tab.path) continue + effect = strongerEntityEffect(effect, m.effect) + } + return effect +} + +/** What a write actually asks of a hosted editor. A write normally needs nothing + * — the editor holds the draft cell it seeds — but `UserDraft.seed` no-ops when + * no editor holds that cell yet, which is the case while one is still loading. + * The write is then only on the server, and the editor has to re-read it: its own + * loader fetches the draft, so a refresh reconciles it. */ +export function effectForWrite( + effect: EntityToolEffect, + seedReachedEditor: boolean +): EntityToolEffect { + return effect === 'none' && !seedReachedEditor ? 'refresh' : effect +} + +/** What a discard asks of a hosted editor. Discarding a draft that had a deployed + * item under it reverts the editor to that item, which a refresh shows — but a + * draft-only item has nothing to revert to, so the discard removed it and the + * editor has to leave rather than remount into a load that cannot resolve. */ +export function effectForDiscard( + effect: EntityToolEffect, + itemSurvives: boolean +): EntityToolEffect { + return effect === 'refresh' && !itemSurvives ? 'close' : effect +} + +/** The stronger of two effects, for a tab several of a round's mutations reach: + * a delete outranks a refresh, which outranks nothing. */ +export function strongerEntityEffect(a: EntityToolEffect, b: EntityToolEffect): EntityToolEffect { + if (a === 'close' || b === 'close') return 'close' + if (a === 'refresh' || b === 'refresh') return 'refresh' + return 'none' +} + function pagesForItemType(type: unknown, args: any): string[] { switch (type) { case 'schedule': @@ -63,10 +172,12 @@ function triggerPages(kind: unknown): string[] { return page ? [page.path] : [] } -// The open tabs a page-reload should refresh: those whose observed page path is -// in `pages`. Item-editor and pipeline tab routes are never list pages, so they -// never match (see the self-sync invariant above). Pure over a tab snapshot so -// the sessions page can reload by id and this stays unit-testable. +// The open tabs a round touched: those whose observed page path is in `pages`. +// Item-editor and pipeline routes are never list pages, so they never match. An +// entity-editor tab does — its location is a list page with the row in the hash, +// which the path comparison drops — and the caller tells the two apart, reloading +// the list frames and passing the rest through `entityEffectForTab`. Pure over a +// tab snapshot so the sessions page can act by id and this stays unit-testable. export function tabsToReload( tabs: SessionPreviewTab[], pages: ReadonlySet diff --git a/frontend/src/lib/components/sessions/previewRouter.test.ts b/frontend/src/lib/components/sessions/previewRouter.test.ts index 7d56f9e717..a35951ccff 100644 --- a/frontend/src/lib/components/sessions/previewRouter.test.ts +++ b/frontend/src/lib/components/sessions/previewRouter.test.ts @@ -14,7 +14,9 @@ import { previewLocationContext, previewLocationLabel, resolvePreviewTab, - runFormUrl + runFormUrl, + entityListHref, + entityEditorHref } from './previewRouter' describe('drawerAnchorFor', () => { @@ -293,6 +295,32 @@ describe('resolvePreviewTab', () => { expect(resolvePreviewTab('/apps/edit/f/a/b')).toEqual({ kind: 'iframe' }) }) + // The row in the hash is what separates "edit this entity" from "show me the + // list": both are the same page path, and only the first can mount in process. + it('routes an anchored row to its in-process entity editor', () => { + expect(resolvePreviewTab('/schedules#u/me/daily')).toEqual({ + kind: 'entity', + entityKind: 'trigger_schedule', + path: 'u/me/daily' + }) + expect(resolvePreviewTab('/resources#/resource/u/me/db')).toEqual({ + kind: 'entity', + entityKind: 'resource', + path: 'u/me/db' + }) + expect(resolvePreviewTab('/variables?owner=u#u/me/token')).toEqual({ + kind: 'entity', + entityKind: 'variable', + path: 'u/me/token' + }) + }) + + it('leaves the bare list page, and an anchored page with no hosted editor, on the iframe', () => { + expect(resolvePreviewTab('/schedules')).toEqual({ kind: 'iframe' }) + // Anchored like the three above, but no editor is mounted for it yet. + expect(resolvePreviewTab('/kafka_triggers#f/team/ingest')).toEqual({ kind: 'iframe' }) + }) + it('routes a pipeline folder to the pipeline editor kind', () => { expect(resolvePreviewTab('/pipeline/my_folder')).toEqual({ kind: 'editor', @@ -416,3 +444,20 @@ describe('run form route', () => { expect(parseRunFormRoute('artifact:abc#Plan')).toBeNull() }) }) + +describe('entity location helpers', () => { + // The list a row was opened from is part of where the tab came from: dropping + // its filters on the way back (or on a delete) lands the user on a list they + // never chose. + it('keeps the list query when going back and when following a rename', () => { + const loc = '/resources?filter_path_of=db#/resource/u/me/a' + expect(entityListHref(loc)).toBe('/resources?filter_path_of=db') + expect(entityEditorHref(loc, 'u/me/b')).toBe('/resources?filter_path_of=db#/resource/u/me/b') + }) + + it('addresses a row the way its own page does', () => { + // Resources route theirs through an extra segment; the others name the path. + expect(entityEditorHref('/schedules#u/me/a', 'u/me/b')).toBe('/schedules#u/me/b') + expect(entityEditorHref('/variables#u/me/a', 'u/me/b')).toBe('/variables#u/me/b') + }) +}) diff --git a/frontend/src/lib/components/sessions/previewRouter.ts b/frontend/src/lib/components/sessions/previewRouter.ts index 9f312ea389..4634f3264a 100644 --- a/frontend/src/lib/components/sessions/previewRouter.ts +++ b/frontend/src/lib/components/sessions/previewRouter.ts @@ -1,6 +1,7 @@ import { ASSETS_PATH, AUDIT_LOGS_PATH, + drawerHashFor, FOLDERS_PATH, GROUPS_PATH, pageKey, @@ -117,6 +118,65 @@ export function drawerAnchorFor(location: string): string | undefined { return location.slice(hashAt + 1).replace(/^\/resource\//, '') || undefined } +/** A workspace entity whose editor the preview hosts in process. The + * `UserDraftItemKind` the entity's draft lives under, so the mounted editor and + * the chat address the same cell. */ +export type EntityEditorKind = 'trigger_schedule' | 'resource' | 'variable' + +// The workspace entities whose single-item editor the preview mounts in process, +// by the list page that deep-links them. Mounting in process is what puts the +// editor in the same realm as the chat: it then holds a live `UserDraft` handle +// on the same cell the chat's writes seed, and reflects them without a reload +// (an iframe has a `UserDraft` of its own, so its only route is `toolReloadEffect`). +// A page absent here keeps loading its whole list in an iframe. +const IN_REALM_ENTITY_PAGES: Partial> = { + [SCHEDULES_PATH]: 'trigger_schedule', + [RESOURCES_PATH]: 'resource', + [VARIABLES_PATH]: 'variable' +} + +/** The entity kind a list page hosts an editor for, or undefined for a page with + * none. The kind doubles as the `UserDraftItemKind` its draft lives under. */ +export function entityKindForPage(pagePath: string): EntityEditorKind | undefined { + return IN_REALM_ENTITY_PAGES[pagePath] +} + +/** The list an entity editor's tab came from: its own location with the row + * dropped. Built from the location rather than the page's bare path so the query + * survives — a row opened from a filtered list returns to that same filtered + * list, whether by Back or because the item was deleted. */ +export function entityListHref(location: string): string { + return location.split('#')[0] +} + +/** The location of `path`'s editor on the list `location` came from, keeping that + * list's query. For following a rename: the tab has to name the item it now + * shows, or the chat's "edit this" and the draft key still point at the old one. */ +export function entityEditorHref(location: string, path: string): string { + return `${entityListHref(location)}#${drawerHashFor(stripBase(location), path)}` +} + +/** The list page an entity editor was reached through, so its host can offer the + * way back. Undefined only if a kind is registered above without a curated page, + * which `PREVIEW_PAGES` covers for all three today. */ +export function entityListPage(kind: EntityEditorKind): PreviewPage | undefined { + const path = Object.keys(IN_REALM_ENTITY_PAGES).find((p) => IN_REALM_ENTITY_PAGES[p] === kind) + return path ? matchPreviewPage(path) : undefined +} + +/** The workspace entity a preview location opens the editor of — a list page + * with a row deep-linked in its hash, for the pages whose editor mounts in + * process. Undefined for the bare list page (an iframe of the list) and for + * anchored pages whose editor is not hosted yet. */ +export function parseEntityEditorRoute( + location: string +): { entityKind: EntityEditorKind; path: string } | undefined { + const entityKind = IN_REALM_ENTITY_PAGES[stripBase(location)] + if (!entityKind) return undefined + const path = drawerAnchorFor(location) + return path ? { entityKind, path } : undefined +} + // Query params the preview host injects into an iframe URL (`nomenubar` hides the nav, // `workspace` scopes the page). Never part of what a location means. const INJECTED_PARAMS = ['nomenubar', 'workspace'] as const @@ -490,6 +550,7 @@ export const isArtifactKey = (key: string) => key.startsWith('artifact:') // other route) stays an iframe. export type PreviewSlot = | { kind: 'editor'; editorKind: SessionTargetKind | 'pipeline'; path: string } + | { kind: 'entity'; entityKind: EntityEditorKind; path: string } | { kind: 'artifact'; id: string; version?: number } | { kind: 'runform'; toolCallId: string } | { kind: 'iframe' } @@ -503,6 +564,10 @@ export function resolvePreviewTab(url: string): PreviewSlot { if (pipelineFolder) { return { kind: 'editor', editorKind: 'pipeline', path: pipelineFolder } } + // Before the item route: an entity location is a list page (never an item + // route) with a row in its hash, so the parse below would call it an iframe. + const entity = parseEntityEditorRoute(url) + if (entity) return { kind: 'entity', ...entity } const route = parsePreviewItemRoute(url) if (!route) return { kind: 'iframe' } const editorKind: SessionTargetKind | undefined = diff --git a/frontend/src/lib/components/sessions/sessionPreviewTabs.svelte.ts b/frontend/src/lib/components/sessions/sessionPreviewTabs.svelte.ts index a9db01e0fa..3f943869d5 100644 --- a/frontend/src/lib/components/sessions/sessionPreviewTabs.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionPreviewTabs.svelte.ts @@ -7,6 +7,7 @@ import { canonicalizeObservedLoc, describeLocation, matchPreviewPage, + parseEntityEditorRoute, showsView, parseArtifactRoute, parsePipelineRoute, @@ -303,6 +304,18 @@ export class SessionPreviewTabs { if ((commandUnchanged && drifted) || fragmentOnly) this.pulseReload(tab.id) } + /** Re-point one tab by id. `navigate` moves whichever tab is active, which is + * what a user's own pick means; this is for a change that belongs to a + * particular tab whether or not the user is looking at it — the item a hosted + * entity editor was showing has been deleted out from under it. Works on a + * tab with no mounted host, which would otherwise never hear about it. */ + retargetTabTo(id: string, url: string): void { + const tab = this.#tabs.find((t) => t.id === id) + if (!tab) return + this.#retarget(tab, url) + this.#flush() + } + // Force the host to reload the iframe. A navigation onto the tab's exact current URL // changes nothing, so URL-driven behavior — a `#` opening a drawer the user has // since closed — would never re-fire. @@ -622,6 +635,14 @@ export class SessionPreviewTabs { const observed = describeLocation(canonical) if (commanded.anchor && !observed.anchor && commanded.identity === observed.identity) { t.url = t.url.split('#')[0] + } else if (!commanded.anchor && observed.anchor && parseEntityEditorRoute(canonical)) { + // The mirror case, for the entities whose editor this panel hosts in + // process: a drawer opened on a row inside the frame is that entity's + // editor, and the frame's copy is a realm apart from the chat's writes — + // it can only be brought up to date by reloading the whole page under it. + // Following the command re-resolves the tab onto the hosted editor, which + // shares the draft cell the chat writes and so tracks it as the user reads. + t.url = canonical } this.#flush() } diff --git a/frontend/src/lib/components/sessions/sessionPreviewTabs.test.ts b/frontend/src/lib/components/sessions/sessionPreviewTabs.test.ts index b71de62c23..2beb6af7b1 100644 --- a/frontend/src/lib/components/sessions/sessionPreviewTabs.test.ts +++ b/frontend/src/lib/components/sessions/sessionPreviewTabs.test.ts @@ -9,7 +9,7 @@ import { type PreviewTabsAdapter, type PreviewTabsSnapshot } from './sessionPreviewTabs.svelte' -import { artifactUrl, type PreviewTarget } from './previewRouter' +import { artifactUrl, resolvePreviewTab, type PreviewTarget } from './previewRouter' import type { SessionPreviewTab } from './sessionState.svelte' import { base } from '$lib/base' @@ -365,6 +365,34 @@ describe('SessionPreviewTabs.open', () => { expect(o.tabs[0].url).toBe('/apps/edit/u/me/dash') }) + // Opening a row inside the list frame is the user asking to edit that entity. The + // frame's copy of the editor is a realm apart from the chat's writes, so the tab + // follows onto the hosted one, which shares the draft cell those writes land in. + it('follows the command onto the hosted editor when a row is opened inside the frame', () => { + const o = owner() + o.open({ type: 'page', href: '/resources', label: 'Resources' }) + const id = o.tabs[0].id + o.observeLocation(id, '/resources#/resource/u/me/db') + + expect(o.tabs[0].url).toBe('/resources#/resource/u/me/db') + expect(resolvePreviewTab(o.tabs[0].url)).toEqual({ + kind: 'entity', + entityKind: 'resource', + path: 'u/me/db' + }) + }) + + // Anchored the same way, but nothing hosts its editor yet: re-commanding would + // point the tab at a row the frame is already showing, for no gain. + it('leaves the command alone for an anchored page with no hosted editor', () => { + const o = owner() + o.open({ type: 'page', href: '/kafka_triggers', label: 'Kafka triggers' }) + const id = o.tabs[0].id + o.observeLocation(id, '/kafka_triggers#f/team/ingest') + + expect(o.tabs[0].url).toBe('/kafka_triggers') + }) + // Re-commanding the URL a tab is already pointed at changes nothing the host can // see, so the frame would stay wherever the user navigated it inside the page. it('forces a reload when the request matches the command but the frame drifted', () => { diff --git a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts index 4c70c31ef4..50adbce520 100644 --- a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts @@ -389,7 +389,12 @@ function createRuntime(session: Session): SessionRuntime { // point those at a page the user cannot see. const tab = owner?.displayedTab if (!tab) return undefined - if (resolvePreviewTab(tab.url).kind !== 'iframe') return undefined + // Entity editors are hosted in process but, unlike the item editors, they + // register no live-editor draft — so this is the only thing that tells the + // model which schedule/resource/variable is on screen, and "change this + // one" would otherwise resolve against nothing. + const kind = resolvePreviewTab(tab.url).kind + if (kind !== 'iframe' && kind !== 'entity') return undefined return previewLocationContext(whereIs(tab)) } // Pre-flight: materialise the (still-transient) session, then commit diff --git a/frontend/src/lib/components/usePageDraftSync.svelte.ts b/frontend/src/lib/components/usePageDraftSync.svelte.ts index 8ca2b0beee..82195806b3 100644 --- a/frontend/src/lib/components/usePageDraftSync.svelte.ts +++ b/frontend/src/lib/components/usePageDraftSync.svelte.ts @@ -97,7 +97,7 @@ export function usePageDraftSync(opts: PageDraftSyncOptions): Pa const ws = opts.workspace() const p = opts.path() if (!ws || !p) return - UserDraft.seed(opts.itemKind, p, value, { workspace: ws }) + UserDraft.seed(opts.itemKind, p, value, { workspace: ws, baseline: true }) }, recordRemoteSync(draftSavedAt: string | undefined) { const ws = opts.workspace() diff --git a/frontend/src/lib/userDraft.svelte.ts b/frontend/src/lib/userDraft.svelte.ts index aa55b2907d..9d3d58523e 100644 --- a/frontend/src/lib/userDraft.svelte.ts +++ b/frontend/src/lib/userDraft.svelte.ts @@ -1,8 +1,9 @@ import { get } from 'svelte/store' +import { SvelteMap } from 'svelte/reactivity' import { onDestroy, untrack } from 'svelte' import { deepEqual } from 'fast-equals' import { workspaceStore } from './stores' -import { readFieldsRecursively } from './utils' +import { readFieldsRecursively, sendUserToast } from './utils' import { UserDraftDbSyncer } from './userDraftDbSyncer.svelte' import type { UserDraftItemKind } from './gen' @@ -124,6 +125,87 @@ export type ClearLiveEditorDraftOptions = UserDraftOptions & { } const entries = new Map() +// Cells whose last `seed` found no live entry (see `takeSeedMiss`). Bounded by +// the seeds that missed and not yet been read back, and each entry is one key. +const seedMisses = new Set() +// Cells whose last discard removed the item itself (see `takeDraftOnlyDiscard`). +const draftOnlyDiscards = new Set() + +// Nothing reads either set outside a session, so unread markers would accumulate. +// Capped by dropping the oldest: a marker means something only to the action that +// immediately follows the write that left it. +const MAX_WRITE_MARKERS = 64 +function noteMarker(set: Set, 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) + } +} +/** + * What a cell held when its last holder let go, recorded only while a write it + * could be newer than is in flight (see `beginDraftSettleWindow`). Outside one a + * release is an editor simply closing, and remembering its value would retain a + * whole resource — or a variable's decrypted secret — for the tab's lifetime. + */ +const releasedValues = new Map() +/** Keys with a write in flight, by how many are. Reactive: it is also what the + * editors disable their Save on. */ +const settleWindows = new SvelteMap() + +/** + * Whether a write is in flight for this cell — in any editor holding it, not just + * the one asking. Duplicate tabs and warm sessions mount several editors over one + * (workspace, kind, path), so a flag kept per editor serializes nothing: two can + * be writing the same item at once, and the older request landing last wins. + */ +export function isDraftSaving( + itemKind: UserDraftItemKind, + path: string | undefined, + opts?: UserDraftOptions +): boolean { + if (!path) return false + return settleWindows.has(mapKey(resolveWorkspace(opts), itemKind, path)) +} + +/** + * Open the window in which a release of this cell is worth remembering: the editor + * stays editable while its save is in flight, so a tab left then hands the settle + * that follows a cell nothing holds any more. Pair with the returned disposer. + */ +export function beginDraftSettleWindow( + itemKind: UserDraftItemKind, + path: string, + opts?: UserDraftOptions +): () => void { + const mk = mapKey(resolveWorkspace(opts), itemKind, path) + settleWindows.set(mk, (settleWindows.get(mk) ?? 0) + 1) + return () => { + const left = (settleWindows.get(mk) ?? 0) - 1 + if (left > 0) { + settleWindows.set(mk, left) + return + } + settleWindows.delete(mk) + // Nothing is left to read it: the settle it was recorded for is over. + releasedValues.delete(mk) + } +} +/** + * Every value the cell was let go of during the window. All of them: an item + * reopened and closed again releases a freshly loaded baseline, and keeping only + * the last would let it erase the edit before it. Only reads — the window owns + * their lifetime, and a second save settling the cell must see what the first saw. + */ +function releasedDuringWindow( + itemKind: UserDraftItemKind, + path: string, + opts?: UserDraftOptions +): V[] { + return (releasedValues.get(mapKey(resolveWorkspace(opts), itemKind, path)) ?? []) as V[] +} + const liveEditorDrafts = new Map() /** @@ -280,6 +362,92 @@ export function draftValuesEqual(a: unknown, b: unknown): boolean { return deepEqual(normalizeDraftForCompare(a), normalizeDraftForCompare(b)) } +/** + * Settle a draft cell after the write of `written` — the value actually sent — + * landed. A form stays editable while its request is in flight, so `live` may + * hold a newer edit; that is a change on top of the save rather than part of it, + * and resetting the cell would swallow it silently. A save that also moved the + * item is the exception: the edit cannot follow (a freshly acquired cell reads + * only its own default), so the cell is reset rather than left orphaned under a + * path the item no longer occupies. + */ +export async function settleDraftAfterWrite( + itemKind: UserDraftItemKind, + written: V, + live: V | undefined, + fromPath: string, + savedPath: string, + opts?: UserDraftOptions +): Promise { + // A create has nothing to settle: the blank form's cell is the detached handle + // `useMany` hands an empty path, wired to no key, so every request made for one + // is addressed to `/drafts/update//` and 404s. + if (!fromPath) return + // Anything this cell held during the write counts, not just what it holds now: + // an editor that let go mid-write and was reopened before this ran leaves a + // freshly loaded entry standing over the edit it released, and reading only the + // live one would take that baseline for "nothing newer" and delete the edit. + const newer = (v: V | undefined) => v !== undefined && !draftValuesEqual(v, written) + const diverged = + newer(live) || releasedDuringWindow(itemKind, fromPath, opts).some((v) => newer(v)) + if (savedPath === fromPath && diverged) return + // The form stays editable across the delete's own request, so a keystroke made + // then queues a write behind it: under an unchanged path that write is the + // user's and stands, but under a path the item has left it would put the draft + // back where no editor is. Suspending that key is what makes one attempt final. + const moved = savedPath !== fromPath + if (moved) UserDraft.stopSync(itemKind, fromPath, opts) + try { + // Sent before this resolves, not left on the keystroke debounce: callers report + // the write and remount on it, and both read a cell this has to have finished + // resolving — including through a frame with a draft store of its own. + UserDraft.discard(itemKind, fromPath, written, opts) + if (await flushDraftDelete(itemKind, fromPath, opts)) return + } finally { + if (moved) UserDraft.restartSync(itemKind, fromPath, opts) + } + if (!moved) return + // A conflict or a failed request, which no retry changes: what is left is a draft + // at a path nothing is editing. Callers still follow the rename — the item is at + // `savedPath`, and leaving them behind would strand the editor too — so this is + // the only thing that says the leftover is there. + sendUserToast(`Saved, but the draft left at ${fromPath} could not be cleared`, true) +} + +/** + * Send whatever this cell has parked on the autosave debounce, now. For a caller + * that has to know the cell is resolved before it acts — settling after a write, + * before anything reports or remounts on it. + */ +export async function flushDraftWrites( + itemKind: UserDraftItemKind, + path: string, + opts?: UserDraftOptions +): Promise { + await UserDraftDbSyncer.flush({ workspace: resolveWorkspace(opts), itemKind, path }) +} + +/** + * Wait for a queued draft delete to land, reporting whether the draft is gone. A + * caller acts on this — a host leaves an editor whose item the discard removed — + * so both halves have to hold: the delete is the last thing that landed, and + * nothing is queued behind it. The form stays editable after Discard, and a + * `flush` only submits the payload it read when it started, so an edit made in + * between rides the debouncer and would recreate the draft after the caller left. + */ +export async function flushDraftDelete( + itemKind: UserDraftItemKind, + path: string, + opts?: UserDraftOptions +): Promise { + const query = { workspace: resolveWorkspace(opts), itemKind, path } + await flushDraftWrites(itemKind, path, opts) + return ( + UserDraftDbSyncer.lastLandedWasDelete(query) && + UserDraftDbSyncer.getState(query).state === 'none' + ) +} + export type UserDraftHandle = { get draft(): V | undefined set draft(value: V | undefined) @@ -290,6 +458,10 @@ export const UserDraft = { const ws = resolveWorkspace(opts) if (liveItems?.seed(ws, itemKind, path, value)) return const mk = mapKey(ws, itemKind, path) + // The item has a draft again, so it exists again: a marker still standing + // (nothing consumed it, because nothing was listening) describes an item this + // write replaced, and would send the next editor to touch it away. + draftOnlyDiscards.delete(mk) const entry = entries.get(mk) if (entry) { // The reactive effect in `acquireEntry` observes this write @@ -464,18 +636,79 @@ export const UserDraft = { * is still needed when a write fans out across components (e.g. an * editor's `initContent` cascading into the bound value). * - * No-op if the entry isn't live yet (acquire via `use`/`useMany` first). + * No-op if the entry isn't live yet (acquire via `use`/`useMany` first) — + * recorded for {@link takeSeedMiss}, since the value then reached the server + * without reaching the editor that will show it. */ - seed(itemKind: UserDraftItemKind, path: string, value: V, opts?: UserDraftOptions): void { + seed( + itemKind: UserDraftItemKind, + path: string, + value: V, + opts?: UserDraftOptions & { + /** This seed carries what the editor just loaded, not a new value — so it + * says nothing about a write that missed the cell before the editor had it, + * and may well be older than one. Such a seed leaves the miss standing. */ + baseline?: boolean + } + ): void { const ws = resolveWorkspace(opts) if (liveItems?.seed(ws, itemKind, path, value)) return const mk = mapKey(ws, itemKind, path) + // A real write gives the item a draft again whether or not an editor is holding + // the cell, so it voids a removal recorded for it either way. + if (!opts?.baseline) draftOnlyDiscards.delete(mk) const entry = entries.get(mk) - if (!entry) return + if (!entry) { + if (!opts?.baseline) noteMarker(seedMisses, mk) + return + } + if (!opts?.baseline) seedMisses.delete(mk) entry.seedNextWrite = true entry.state.val = snapshotDraftValue(value) }, + /** + * Record that the draft just discarded for this cell was the item's only stored + * form — nothing deployed underneath, so the item is now gone rather than + * reverted. Read once by a consumer showing that item, which has to stop + * showing it. Set at the discard, where the deployed side is known. + */ + recordDraftOnlyDiscard(itemKind: UserDraftItemKind, path: string, opts?: UserDraftOptions): void { + noteMarker(draftOnlyDiscards, mapKey(resolveWorkspace(opts), itemKind, path)) + }, + + /** + * The item at this path is there, whatever an earlier discard recorded. Nothing + * consumes a marker outside a session, so one can outlive the item it was about + * — recreated from another tab, another client — and be spent by the next action + * on the item that took its place. + */ + clearDraftOnlyDiscard(itemKind: UserDraftItemKind, path: string, opts?: UserDraftOptions): void { + draftOnlyDiscards.delete(mapKey(resolveWorkspace(opts), itemKind, path)) + }, + + /** Whether the last discard for this cell removed the item outright (see + * {@link recordDraftOnlyDiscard}), clearing the record. */ + takeDraftOnlyDiscard( + itemKind: UserDraftItemKind, + path: string, + opts?: UserDraftOptions + ): boolean { + return draftOnlyDiscards.delete(mapKey(resolveWorkspace(opts), itemKind, path)) + }, + + /** + * Whether the last {@link seed} for this cell found no live entry, clearing the + * record. An editor acquires its cell only once its first load resolves, so a + * seed during that window reaches nothing and the value lands on the server + * alone — a caller showing that editor has to make it re-read. Recorded at the + * seed rather than asked afterwards: by then the editor may have acquired the + * cell, hiding the very miss this reports. + */ + takeSeedMiss(itemKind: UserDraftItemKind, path: string, opts?: UserDraftOptions): boolean { + return seedMisses.delete(mapKey(resolveWorkspace(opts), itemKind, path)) + }, + /** * Currently-mounted live entries for `workspace` (in-tab only — for a * workspace-wide view call `DraftService` directly). @@ -906,6 +1139,10 @@ function acquireEntry( // stays armed: an undefined-seeded cell's initial run lands // here, and page editors rely on it to swallow their load write.) if (entry.seedNextWrite) entry.seedNextWrite = false + // Same for a `discard`/`remove` whose fallback equals what the cell + // already holds — settling a save that changed nothing does exactly + // that — which otherwise leaves the guard armed for the next edit. + if (entry.skipNextSync) entry.skipNextSync = false return } lastSerialized = next @@ -934,6 +1171,8 @@ function acquireEntry( // copy. `untrack` so reactive reads in the predicate (the editor's // post-deploy baseline) don't re-fire the mirror. const atBaseline = untrack(() => val !== undefined && (discardIf?.(val) ?? false)) + // Same as `save`: a value persisted here means the item exists again. + if (val !== undefined && !atBaseline) draftOnlyDiscards.delete(mk) void UserDraftDbSyncer.save({ workspace, itemKind, @@ -963,6 +1202,12 @@ function releaseEntry(mk: string): void { // only here, once, at refcount 0. This is what lets multiple holders (warm // session previews + the nav editor) share the entry and drop in any order. if (entry.count <= 0) { + if (settleWindows.has(mk)) { + releasedValues.set(mk, [ + ...(releasedValues.get(mk) ?? []), + snapshotDraftValue(entry.state.val) + ]) + } // The live entry was authoritative while mounted; once gone, drop any // cached write for this key so a later read falls back to the server // rather than a value the editor may have changed in the meantime. diff --git a/frontend/src/lib/userDraftDbSyncer.svelte.ts b/frontend/src/lib/userDraftDbSyncer.svelte.ts index 0e8257023d..85842ca881 100644 --- a/frontend/src/lib/userDraftDbSyncer.svelte.ts +++ b/frontend/src/lib/userDraftDbSyncer.svelte.ts @@ -228,6 +228,17 @@ const failures = new SvelteMap() */ const flushes = new SvelteMap() +/** + * Whether the last save that LANDED for a key deleted the draft or wrote one. + * Written only by the response handler, which is what distinguishes it from the + * display-level draft hint: that one is also published optimistically and by the + * editors themselves, so it cannot say whether a POST succeeded. A caller acting + * on a delete (leaving an editor whose item it removed) needs the difference. + * Dropped when the server stops being ours to describe — a conflict, or a resync + * from a load — and otherwise kept, since the key's own next landing replaces it. + */ +const lastLanded = new Map() + /** * Per-key listeners fired when a save for that key LANDS on the server * (`status === 'saved'` with a non-null value — the draft now exists @@ -303,6 +314,7 @@ async function postSave(opts: UserDraftDbSyncerSaveOpts): Promise { serverTimestamp: resp.current_timestamp, localLastSync: lastSync ?? null }) + lastLanded.delete(key) return } // resp.status === 'saved' — advance lastSync (or drop on delete). @@ -316,6 +328,7 @@ async function postSave(opts: UserDraftDbSyncerSaveOpts): Promise { // (value !== null → exists). Every delete path clears the hint for // free instead of maintaining a separate source of truth. setLocalDraftHint(opts.workspace, opts.itemKind, opts.path, opts.value !== null) + lastLanded.set(key, opts.value === null ? 'delete' : 'upsert') conflicts.delete(key) failures.delete(key) // Clear pending only if it's still the opts we just saved — a @@ -535,9 +548,11 @@ export const UserDraftDbSyncer = { } else { clearLastSync(query.workspace, query.itemKind, query.path) } - // Back in sync with the server: clear any conflict / failure. + // Back in sync with the server: clear any conflict / failure. `lastLanded` + // goes with them — whatever we last wrote no longer describes what is there. conflicts.delete(key) failures.delete(key) + lastLanded.delete(key) }, /** @@ -593,6 +608,16 @@ export const UserDraftDbSyncer = { } }, + /** + * Whether the last save that landed for this key was the draft's deletion. + * False while none has landed: a delete that failed or conflicted never + * reaches the response handler, and a conflict drops what an earlier one + * recorded rather than letting it answer for this attempt. + */ + lastLandedWasDelete(query: UserDraftLastSyncQuery): boolean { + return lastLanded.get(draftKey(query.workspace, query.itemKind, query.path)) === 'delete' + }, + /** Reactive conflict snapshot (if any) for a draft. */ getConflict(query: UserDraftLastSyncQuery): { readonly conflict: DraftConflictInfo | undefined diff --git a/frontend/src/lib/userDraftFlushDelete.test.ts b/frontend/src/lib/userDraftFlushDelete.test.ts new file mode 100644 index 0000000000..5868cd581a --- /dev/null +++ b/frontend/src/lib/userDraftFlushDelete.test.ts @@ -0,0 +1,60 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest' + +// Stands in for the syncer's per-key facts. `landed` is what the response +// handler recorded; `state` is what is still queued behind it. +let landedDelete = false +let state: 'none' | 'pending' | 'saving' | 'failed' = 'none' +vi.mock('./userDraftDbSyncer.svelte', () => ({ + UserDraftDbSyncer: { + flush: vi.fn(async () => {}), + lastLandedWasDelete: vi.fn(() => landedDelete), + getState: vi.fn(() => ({ state })), + save: vi.fn() + } +})) +vi.mock('./gen', () => ({ DraftService: { updateDraft: vi.fn() } })) +vi.mock('./gen/core/OpenAPI', () => ({ OpenAPI: { BASE: '' } })) +vi.mock('./localDraftHints.svelte', () => ({ setLocalDraftHint: vi.fn() })) + +import { flushDraftDelete } from './userDraft.svelte' + +const run = () => flushDraftDelete('variable', 'u/me/v', { workspace: 'ws' }) + +beforeEach(() => { + landedDelete = false + state = 'none' +}) + +/** + * A caller leaves an editor on this verdict, so anything short of "the draft is + * gone and staying gone" has to read false. + */ +describe('flushDraftDelete', () => { + it('confirms a delete that landed with nothing behind it', async () => { + landedDelete = true + expect(await run()).toBe(true) + }) + + // The form stays editable after Discard and `flush` only submits what it read + // when it started, so an edit made in between is still queued — and would + // recreate the draft once the caller had already left. + it('rejects a delete with an edit queued behind it', async () => { + landedDelete = true + state = 'pending' + expect(await run()).toBe(false) + }) + + // A failed delete never reaches the response handler, so nothing records it as + // landed — the display hint would have said otherwise, since the editor + // publishes that one itself the moment the cell returns to its baseline. + it('rejects a delete that failed', async () => { + landedDelete = false + state = 'failed' + expect(await run()).toBe(false) + }) + + it('rejects a key where an upsert landed last', async () => { + landedDelete = false + expect(await run()).toBe(false) + }) +}) diff --git a/frontend/src/lib/userDraftLastLanded.test.ts b/frontend/src/lib/userDraftLastLanded.test.ts new file mode 100644 index 0000000000..fd030d79b0 --- /dev/null +++ b/frontend/src/lib/userDraftLastLanded.test.ts @@ -0,0 +1,37 @@ +import { describe, it, expect, vi } from 'vitest' + +let response: { status: 'saved' | 'conflict'; current_timestamp: string } = { + status: 'saved', + current_timestamp: '2020-01-01T00:00:00Z' +} +const updateDraft = vi.fn(async (..._args: any[]) => response) +vi.mock('./gen', () => ({ + DraftService: { updateDraft: (...a: unknown[]) => updateDraft(...(a as [])) } +})) +vi.mock('./gen/core/OpenAPI', () => ({ OpenAPI: { BASE: '' } })) +vi.mock('./localDraftHints.svelte', () => ({ setLocalDraftHint: vi.fn() })) + +import { UserDraftDbSyncer } from './userDraftDbSyncer.svelte' + +const query = { workspace: 'ws', itemKind: 'variable' as const, path: 'u/me/v' } +const send = (value: unknown) => UserDraftDbSyncer.save({ ...query, value, immediate: true }) + +/** + * `flushDraftDelete` leaves an editor on this fact, and it is deliberately + * sticky — so it has to stop describing the server the moment the server stops + * being ours to describe. + */ +describe('lastLandedWasDelete', () => { + it('follows what the response handler saw land, and drops it on a conflict', async () => { + await send({ path: 'u/me/v' }) + expect(UserDraftDbSyncer.lastLandedWasDelete(query)).toBe(false) + + await send(null) + expect(UserDraftDbSyncer.lastLandedWasDelete(query)).toBe(true) + + // The server moved under us: the earlier delete no longer answers for this one. + response = { status: 'conflict', current_timestamp: '2020-01-02T00:00:00Z' } + await send(null) + expect(UserDraftDbSyncer.lastLandedWasDelete(query)).toBe(false) + }) +}) diff --git a/frontend/src/lib/userDraftSeedMiss.test.ts b/frontend/src/lib/userDraftSeedMiss.test.ts new file mode 100644 index 0000000000..6369a69ec5 --- /dev/null +++ b/frontend/src/lib/userDraftSeedMiss.test.ts @@ -0,0 +1,45 @@ +import { describe, it, expect, vi } from 'vitest' + +vi.mock('./gen', () => ({ DraftService: { updateDraft: vi.fn() } })) +vi.mock('./gen/core/OpenAPI', () => ({ OpenAPI: { BASE: '' } })) +vi.mock('./localDraftHints.svelte', () => ({ setLocalDraftHint: vi.fn() })) +vi.mock('./userDraftDbSyncer.svelte', () => ({ + UserDraftDbSyncer: { save: vi.fn(), flush: vi.fn(async () => {}) } +})) + +import { UserDraft } from './userDraft.svelte' + +const OPTS = { workspace: 'ws' } +let n = 0 +const nextPath = () => `u/me/s${n++}` + +/** + * A chat write seeds the cell an editor is about to hold. Seeding before it does + * records the miss, which the session reads back to know the write never reached + * the form and the editor has to re-read it. + */ +describe('seed misses', () => { + it('records a miss when no editor holds the cell', () => { + const path = nextPath() + UserDraft.seed('trigger_schedule', path, { a: 1 }, OPTS) + expect(UserDraft.takeSeedMiss('trigger_schedule', path, OPTS)).toBe(true) + // Read once: the next action must not spend the same marker. + expect(UserDraft.takeSeedMiss('trigger_schedule', path, OPTS)).toBe(false) + }) + + // The editor adopts what it loaded as the cell's baseline once it mounts, and + // that load can predate the write that missed. Such a seed stays out of the + // bookkeeping entirely — it neither records a miss nor consumes one. + it('leaves a standing miss alone when the editor bootstraps its baseline', () => { + const path = nextPath() + UserDraft.seed('trigger_schedule', path, { a: 1 }, OPTS) + UserDraft.seed('trigger_schedule', path, { a: 0 }, { ...OPTS, baseline: true }) + expect(UserDraft.takeSeedMiss('trigger_schedule', path, OPTS)).toBe(true) + }) + + it('records nothing for a bootstrap seed of its own', () => { + const path = nextPath() + UserDraft.seed('trigger_schedule', path, { a: 0 }, { ...OPTS, baseline: true }) + expect(UserDraft.takeSeedMiss('trigger_schedule', path, OPTS)).toBe(false) + }) +}) diff --git a/frontend/src/lib/userDraftSettleAfterWrite.test.ts b/frontend/src/lib/userDraftSettleAfterWrite.test.ts new file mode 100644 index 0000000000..1e92b67423 --- /dev/null +++ b/frontend/src/lib/userDraftSettleAfterWrite.test.ts @@ -0,0 +1,181 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest' + +const discard = vi.fn() +const { toast } = vi.hoisted(() => ({ toast: vi.fn() })) +vi.mock('./utils', async (orig) => ({ ...((await orig()) as object), sendUserToast: toast })) +// Whether the key ends up settled on the delete after a flush — false stands for a +// form that keeps queueing writes behind it. +let settles = true +vi.mock('./gen', () => ({ DraftService: { updateDraft: vi.fn() } })) +vi.mock('./gen/core/OpenAPI', () => ({ OpenAPI: { BASE: '' } })) +vi.mock('./localDraftHints.svelte', () => ({ setLocalDraftHint: vi.fn() })) +vi.mock('./userDraftDbSyncer.svelte', () => ({ + UserDraftDbSyncer: { + save: vi.fn(), + flush: vi.fn(async () => {}), + lastLandedWasDelete: vi.fn(() => settles), + getState: vi.fn(() => ({ state: 'none' })) + } +})) + +import { + beginDraftSettleWindow, + isDraftSaving, + settleDraftAfterWrite, + UserDraft +} from './userDraft.svelte' + +const stopSync = vi.fn() +const restartSync = vi.fn() + +beforeEach(() => { + settles = true + toast.mockClear() + discard.mockClear() + stopSync.mockClear() + restartSync.mockClear() + vi.spyOn(UserDraft, 'discard').mockImplementation(discard as any) + vi.spyOn(UserDraft, 'stopSync').mockImplementation(stopSync as any) + vi.spyOn(UserDraft, 'restartSync').mockImplementation(restartSync as any) +}) + +const OPTS = { workspace: 'ws' } +const sent = { path: 'u/me/a', value: 'sent' } + +// Duplicate tabs and warm sessions mount several editors over one cell, so what +// says "a write is in flight for this item" cannot live in any of them. The +// window each save opens is per key, and every editor's Save reads it. +describe('isDraftSaving', () => { + it('answers for the cell rather than the editor, until the last save closes', () => { + expect(isDraftSaving('variable', 'u/me/a', OPTS)).toBe(false) + const first = beginDraftSettleWindow('variable', 'u/me/a', OPTS) + const second = beginDraftSettleWindow('variable', 'u/me/a', OPTS) + expect(isDraftSaving('variable', 'u/me/a', OPTS)).toBe(true) + expect(isDraftSaving('variable', 'u/me/b', OPTS)).toBe(false) + first() + expect(isDraftSaving('variable', 'u/me/a', OPTS)).toBe(true) + second() + expect(isDraftSaving('variable', 'u/me/a', OPTS)).toBe(false) + }) + + // A create's form cell is routed nowhere, so nothing can collide with it. + it('is false for a path a cell cannot be keyed on', () => { + expect(isDraftSaving('variable', '', OPTS)).toBe(false) + expect(isDraftSaving('variable', undefined, OPTS)).toBe(false) + }) +}) + +/** + * The forms stay editable while their save is in flight, so the cell can hold a + * newer edit by the time the write returns. Resetting it unconditionally — what + * every editor did — swallows that edit with no trace. + */ +describe('settleDraftAfterWrite', () => { + it('resets the cell when it still holds what was written', () => { + settleDraftAfterWrite('variable', sent, { ...sent }, 'u/me/a', 'u/me/a', OPTS) + expect(discard).toHaveBeenCalledWith('variable', 'u/me/a', sent, OPTS) + }) + + it('keeps an edit made while the write was in flight', () => { + settleDraftAfterWrite( + 'variable', + sent, + { path: 'u/me/a', value: 'typed' }, + 'u/me/a', + 'u/me/a', + OPTS + ) + expect(discard).not.toHaveBeenCalled() + }) + + // A renamed save is the exception: a freshly acquired cell reads only its own + // default, so the edit cannot follow the item and the alternative to resetting + // is an orphan draft under a path the item no longer occupies. + it('resets even a diverged cell when the write moved the item', () => { + settleDraftAfterWrite( + 'variable', + sent, + { path: 'u/me/a', value: 'typed' }, + 'u/me/a', + 'u/me/b', + OPTS + ) + expect(discard).toHaveBeenCalledWith('variable', 'u/me/a', sent, OPTS) + }) + + // A rename suspends the old key first, so nothing can queue behind the delete and + // re-sending it could only repeat a request the server already refused. What is + // left is a draft under a path the item has left, which has to be reported. + it('sends the delete once on a rename and reports one that will not land', async () => { + settles = false + await settleDraftAfterWrite('variable', sent, { ...sent }, 'u/me/a', 'u/me/b', OPTS) + expect(discard).toHaveBeenCalledTimes(1) + expect(toast).toHaveBeenCalledTimes(1) + }) + + // A create's form cell is the detached handle an empty path gets, wired to no + // key: every request made for one is addressed to `/drafts/update//`. + it('settles nothing for a create', async () => { + await settleDraftAfterWrite('variable', sent, undefined, '', 'u/me/a', OPTS) + expect(discard).not.toHaveBeenCalled() + }) + + // A released handle reads as no cell at all. That is not somebody's newer edit — + // it is a draft nothing is watching — so the cleanup still has to run. + it('cleans up when the editor has been released', async () => { + await settleDraftAfterWrite('variable', sent, undefined, 'u/me/a', 'u/me/a', OPTS) + expect(discard).toHaveBeenCalledWith('variable', 'u/me/a', sent, OPTS) + }) + + // On an unchanged path an edit made during the delete is the user's: the key is + // left accepting writes, and a draft standing there is the intended outcome + // rather than something to report. + it('leaves an edit made during the delete on an unchanged path', async () => { + settles = false + await settleDraftAfterWrite('variable', sent, { ...sent }, 'u/me/a', 'u/me/a', OPTS) + expect(discard).toHaveBeenCalledTimes(1) + expect(toast).not.toHaveBeenCalled() + }) + + // What makes the single attempt safe: the moved-from key stops accepting writes + // for the length of the delete, so a keystroke cannot queue behind it. Dropping + // the bracket would leave every other case here green. + it('suspends the key it is deleting, and only for a rename', async () => { + await settleDraftAfterWrite('variable', sent, { ...sent }, 'u/me/a', 'u/me/b', OPTS) + expect(stopSync).toHaveBeenCalledWith('variable', 'u/me/a', OPTS) + expect(restartSync).toHaveBeenCalledWith('variable', 'u/me/a', OPTS) + stopSync.mockClear() + restartSync.mockClear() + await settleDraftAfterWrite('variable', sent, { ...sent }, 'u/me/a', 'u/me/a', OPTS) + expect(stopSync).not.toHaveBeenCalled() + expect(restartSync).not.toHaveBeenCalled() + }) + + it('sends it once when the key settles', async () => { + await settleDraftAfterWrite('variable', sent, { ...sent }, 'u/me/a', 'u/me/b', OPTS) + expect(discard).toHaveBeenCalledTimes(1) + }) + + it('compares through the draft normalization, so a nested edit is not missed', () => { + const written = { path: 'u/me/a', args: { host: 'h' } } + settleDraftAfterWrite( + 'resource', + written, + { path: 'u/me/a', args: { host: 'h' } }, + 'u/me/a', + 'u/me/a', + OPTS + ) + expect(discard).toHaveBeenCalledTimes(1) + discard.mockClear() + settleDraftAfterWrite( + 'resource', + written, + { path: 'u/me/a', args: { host: 'edited' } }, + 'u/me/a', + 'u/me/a', + OPTS + ) + expect(discard).not.toHaveBeenCalled() + }) +}) diff --git a/frontend/src/routes/(root)/(logged)/sessions/+page.svelte b/frontend/src/routes/(root)/(logged)/sessions/+page.svelte index 8c38795059..2a9ec87d50 100644 --- a/frontend/src/routes/(root)/(logged)/sessions/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/sessions/+page.svelte @@ -38,7 +38,10 @@ } from '$lib/components/sessions/sessionState.svelte' import { withWorkspaceParam } from '$lib/components/sessions/sessionMode.svelte' import { enterSessionMode } from '$lib/components/sessions/sessionSwitch.svelte' - import type { SessionPreviewTabs } from '$lib/components/sessions/sessionPreviewTabs.svelte' + import { + whereIs, + type SessionPreviewTabs + } from '$lib/components/sessions/sessionPreviewTabs.svelte' import { userStore, userWorkspaces, usersWorkspaceStore, workspaceStore } from '$lib/stores' import { getOrCreateRuntime, @@ -62,11 +65,25 @@ pageKey, parseArtifactRoute, parseRunFormRoute, + entityEditorHref, + entityKindForPage, + entityListHref, + entityListPage, + type EntityEditorKind, + parseEntityEditorRoute, parsePreviewItemRoute, previewLocationLabel, + stripBase, type PreviewTarget } from '$lib/components/sessions/previewRouter' - import { toolReloadEffect, tabsToReload } from '$lib/components/sessions/previewReload' + import { + toolReloadEffect, + tabsToReload, + entityEffectForTab, + effectForWrite, + effectForDiscard, + type EntityMutation + } from '$lib/components/sessions/previewReload' import { leafKeyFor, loadKind, @@ -74,6 +91,8 @@ type WorkspaceItemKind } from '$lib/components/workspacePicker' import { splitterPointerCapture } from '$lib/utils/splitterPointerCapture' + import { UserDraft } from '$lib/userDraft.svelte' + import { UserDraftDbSyncer } from '$lib/userDraftDbSyncer.svelte' const globalEnabled = isGlobalAiEnabled() @@ -559,40 +578,169 @@ const tabHosts: Record = {} let reloadHandle: ReturnType | undefined - // Base-stripped list-page paths (e.g. `/schedules`) a chat round touched since - // the last flush — see toolReloadEffect for how tools map to pages. + /** A list page in the workspace it was touched in — see `queuePageReload`. */ + function pageReloadKey(workspace: string, page: string): string { + return `${workspace}\u0000${page}` + } + // Pages touched since the last flush, by whatever touched them: a chat round + // (see toolReloadEffect), a hosted editor's write, a draft landing after one. let pendingPages = new Set() + // The same round's mutations, kept whole rather than folded into one verdict: + // a hosted entity editor is only affected by a mutation to its own item, in + // its own workspace, so the path and workspace have to survive the debounce. + let pendingMutations: EntityMutation[] = [] // Reload the mounted list-page tabs a chat round changed, across all warm // sessions (a hidden preview would otherwise show pre-mutation content on // return). tabsToReload picks only the tabs whose page is in `pages`. - function reloadTabs(pages: Set) { + function reloadTabs(pages: Set, mutations: EntityMutation[]) { for (const s of warmSessions) { const owner = getRuntime(s.id)?.previewTabs if (!owner) continue - for (const tab of tabsToReload(owner.tabs, pages)) { + const workspace = getEffectiveWorkspaceId(s) + // The queue names the workspace each page was touched in; this session only + // cares about the ones touched in its own. + const prefix = pageReloadKey(workspace ?? '', '') + const mine = new Set( + [...pages].filter((p) => p.startsWith(prefix)).map((p) => p.slice(prefix.length)) + ) + for (const tab of tabsToReload(owner.tabs, mine)) { const key = tabKey(s.id, tab.id) - if (mountedTabKeys.has(key)) tabHosts[key]?.reload() + // A list tab shows every row, so any mutation on its page is its + // business. A hosted entity tab shows one item, and is told apart here. + const entity = parseEntityEditorRoute(whereIs(tab)) + if (!entity) { + if (mountedTabKeys.has(key)) tabHosts[key]?.reload() + continue + } + const listPage = stripBase(whereIs(tab)) + const effect = workspace + ? entityEffectForTab(mutations, { listPage, path: entity.path, workspace }) + : 'none' + // Deletion is re-pointed on the tab model, so a tab whose host is not + // mounted is not left sitting on an item that no longer exists. + // The tab's own location, so the list it came from keeps its filters. + if (effect === 'close') { + owner.retargetTabTo(tab.id, entityListHref(whereIs(tab))) + } else if (effect === 'refresh' && mountedTabKeys.has(key)) { + tabHosts[key]?.reload({ entity: 'refresh' }) + } } } } + // A hosted entity editor wrote its item. Every tab on it hears, across warm + // sessions: they share the one draft cell and the one server row, so one left + // behind edits a path that is gone or discards over a value already deployed. + function entityWritten(ev: { + kind: EntityEditorKind + path: string + workspace: string + /** The path it wrote to; absent when the write removed the item. */ + to?: string + fromSessionId: string | undefined + fromTabId: string + /** Whether the editor that reported is still the one mounted on that tab. */ + fromLive?: boolean + }) { + const page = entityListPage(ev.kind)?.path + if (!page) return + for (const s of warmSessions) { + const owner = getRuntime(s.id)?.previewTabs + if (!owner || getEffectiveWorkspaceId(s) !== ev.workspace) continue + for (const tab of owner.tabs) { + const loc = whereIs(tab) + if (stripBase(loc) !== page) continue + const key = tabKey(s.id, tab.id) + const entity = parseEntityEditorRoute(loc) + // A bare list tab shows every row, so any write on its page is its business + // — same as for a chat mutation, and its rows and `*` markers go stale + // otherwise. Queued rather than reloaded here, so one deploy's writes and + // this report collapse into a single reload. + if (!entity) { + queuePageReload(ev.workspace, page) + continue + } + if (entity.path !== ev.path) continue + // The tab's own location, so the list it came from keeps its filters. + if (!ev.to) owner.retargetTabTo(tab.id, entityListHref(loc)) + else if (ev.to !== ev.path) owner.retargetTabTo(tab.id, entityEditorHref(loc, ev.to)) + // Same path: nothing to re-point, but every OTHER editor on it holds a + // baseline the deploy has replaced and would offer to discard back to it. + // The reporting one settled its own, edit typed mid-write included, which a + // remount would re-read from under — unless it is gone and what replaced it + // loaded before the save landed. + else if (ev.fromLive === false || s.id !== ev.fromSessionId || tab.id !== ev.fromTabId) + tabHosts[key]?.reload({ entity: 'refresh' }) + } + } + } + // One queue for every reason a previewed list page goes stale — a chat tool, a + // hosted editor's write, a draft landing afterwards — so the same frame is not + // reloaded several times for one deploy. Keyed by workspace too: a write in one + // says nothing about the same page in another, and reloading it there costs the + // frame its scroll and everything else it holds. + function queuePageReload(workspace: string | undefined, page: string) { + if (!workspace) return + pendingPages.add(pageReloadKey(workspace, page)) + clearTimeout(reloadHandle) + reloadHandle = setTimeout(flushReload, 500) + } function flushReload() { const pages = pendingPages + const mutations = pendingMutations pendingPages = new Set() - reloadTabs(pages) + pendingMutations = [] + reloadTabs(pages, mutations) } + // A list frame reads its rows and `*` markers from a store inside the iframe, so + // nothing we do afterwards corrects what it already read. Rather than time our + // reloads against writes, reload when one lands: a draft written after a save — + // an edit made while it was in flight — reaches the list this way too. + $effect(() => { + return UserDraftDbSyncer.onAnySaved(({ workspace, itemKind }) => { + const page = entityListPage(itemKind as EntityEditorKind)?.path + if (page) queuePageReload(workspace, page) + }) + }) + $effect(() => { // Debounced so a burst of writes (the AI editing several files) reloads once. - setToolCompletionListener((name, args) => { - const { pages } = toolReloadEffect(name, args) + setToolCompletionListener((name, args, workspace, result) => { + const { pages, entity, path, to } = toolReloadEffect(name, args, result) if (pages.length === 0) return - for (const p of pages) pendingPages.add(p) + for (const p of pages) pendingPages.add(pageReloadKey(workspace, p)) + // A write reaches a hosted editor through the draft cell it holds, but an + // editor still loading holds none yet and the seed no-opped past it. The + // miss is recorded when it happens — asking now would be too late, since + // the editor can have acquired the cell during the write's round-trip. + // Each marker is read only by the kind of tool that writes it. A marker + // left unread — nothing consumes them outside a session — would otherwise + // be spent by whatever touched the item next, and a removal marker read by + // a deploy sends a live editor away. + const kind = path ? entityKindForPage(pages[0]) : undefined + const readable = !!kind && !!path + const seedReachedEditor = + !readable || entity !== 'none' || !UserDraft.takeSeedMiss(kind!, path!, { workspace }) + // A discard of a draft-only item removed the item itself, so its editor has + // nothing left to re-read and must leave instead. + const itemSurvives = + !readable || + name !== 'discard_local_draft' || + !UserDraft.takeDraftOnlyDiscard(kind!, path!, { workspace }) + const effect = effectForDiscard(effectForWrite(entity, seedReachedEditor), itemSurvives) + // A mutation that moved the item is a rename like any other, so it goes + // through the same report an editor's own save makes: the tabs on the old + // path follow it, rather than being refreshed onto one that is now empty. + if (kind && path && to && to !== path) { + entityWritten({ kind, path, to, workspace, fromSessionId: undefined, fromTabId: '' }) + } else if (effect !== 'none') pendingMutations.push({ pages, effect, path, workspace }) clearTimeout(reloadHandle) reloadHandle = setTimeout(flushReload, 500) }) return () => { clearTimeout(reloadHandle) pendingPages = new Set() + pendingMutations = [] setToolCompletionListener(undefined) } }) @@ -732,10 +880,21 @@ return ( tab.friendlyLabel ?? (listed && itemDisplayName(listed.path, listed.draftPath, listed.summary)) ?? + // An entity tab hosts one item's editor; `previewLocationLabel` would name + // it after the list page its location shares a path with, so several open + // at once would all read "Schedules". (Not moved into that function: the + // chat's location context reports the page and the anchored row as + // separate fields, and reads the page name from there.) + entityTabLabel(tab.loc) ?? previewLocationLabel(tab.loc) ) } + function entityTabLabel(loc: string): string | undefined { + const entity = parseEntityEditorRoute(loc) + return entity ? (entity.path.split('/').pop() ?? entity.path) : undefined + } + // Hover title for a tab. A summary label is free text the strip truncates, and // it hides the path entirely, so the tooltip carries both. The path shown is // the item's staged one when it has one — a draft's `…/draft_` storage @@ -1094,6 +1253,7 @@ {fullscreen} onNavigate={navigateEditorTo} onLoad={(frame) => tabs && onTabLoad(tabs, tab, frame)} + onEntityWritten={entityWritten} /> {/each} {/each}