mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-13 16:02:59 +00:00
fix(sessions): rebind hosted entity editors on rescope, rename and save
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KcyBRxE8j3ujCYkum9KrC4
This commit is contained in:
co-authored by
Claude Opus 5
parent
ba1ae123fe
commit
f79160e598
@@ -30,9 +30,10 @@
|
||||
workspace?: string
|
||||
disableChatOffset?: boolean
|
||||
onRestored?: () => void
|
||||
/** Fires after Save has written, for a caller showing state derived from the
|
||||
* resource — `onRestored` only covers restoring an old version. */
|
||||
onSaved?: () => void
|
||||
/** Fires after Save has written, with the path it wrote to — which is not the
|
||||
* one it was opened on when the user renamed it. For a caller showing state
|
||||
* derived from the resource; `onRestored` only covers restoring a version. */
|
||||
onSaved?: (savedPath?: string) => void
|
||||
/**
|
||||
* False renders the editor in place instead of in a drawer, for a host that
|
||||
* gives it a pane of its own (a session's resource tab). Same convention as
|
||||
@@ -64,6 +65,8 @@
|
||||
| undefined = $state(undefined)
|
||||
let hasLocalDraft = $state(false)
|
||||
let canWriteSelected = $state(true)
|
||||
// The path as edited in the form, which a rename moves off `path`.
|
||||
let livePath: string | undefined = $state(undefined)
|
||||
|
||||
let path: string | undefined = $state(undefined)
|
||||
let selected: string | undefined = $state(undefined)
|
||||
@@ -153,6 +156,7 @@
|
||||
bind:canSave
|
||||
bind:selected
|
||||
bind:viewJsonSchema
|
||||
onChange={(e) => (livePath = e.path)}
|
||||
onDraftStateChange={(v) => (hasLocalDraft = v)}
|
||||
onCanWriteChange={(v) => (canWriteSelected = v)}
|
||||
/>
|
||||
@@ -204,7 +208,13 @@
|
||||
const saved = resourceEditor?.save()
|
||||
drawer?.closeDrawer()
|
||||
await saved
|
||||
onSaved?.()
|
||||
// Rendered inline there is no drawer to close, so the mounted editor would
|
||||
// otherwise keep the pre-save baseline. Follow a rename before remounting,
|
||||
// or it comes back up on a path the save just moved the item off.
|
||||
const savedPath = livePath ?? path
|
||||
if (savedPath) path = savedPath
|
||||
editorGeneration++
|
||||
onSaved?.(savedPath)
|
||||
}}
|
||||
disabled={!canSave}
|
||||
>
|
||||
|
||||
@@ -42,7 +42,8 @@
|
||||
let {
|
||||
workspace = undefined,
|
||||
useDrawer = true,
|
||||
onBack = undefined
|
||||
onBack = undefined,
|
||||
onSaved = undefined
|
||||
}: {
|
||||
workspace?: string
|
||||
/**
|
||||
@@ -55,6 +56,9 @@
|
||||
/** Inline only: offered in the header when the host replaced something the
|
||||
* user should be able to get back to (a session tab that took over the list). */
|
||||
onBack?: () => void
|
||||
/** Fires after a save, with the path it wrote to — which is not the one it was
|
||||
* opened on when the user renamed it. */
|
||||
onSaved?: (savedPath?: string) => void
|
||||
} = $props()
|
||||
let curWs = $derived(workspace ?? $workspaceStore)
|
||||
|
||||
@@ -316,7 +320,12 @@
|
||||
}
|
||||
sendUserToast(edit ? `Updated variable in ${dirty.length} workspace(s)` : `Created variable`)
|
||||
dispatch('create')
|
||||
// A rename moved the item; the drawer host closes over it, but an inline one
|
||||
// stays mounted, so follow the new path here and tell the host about it.
|
||||
const savedPath = current?.path ?? editPath
|
||||
if (savedPath && savedPath !== editPath) editPath = savedPath
|
||||
drawer?.closeDrawer()
|
||||
onSaved?.(savedPath)
|
||||
} catch (err) {
|
||||
sendUserToast(`Could not save variable: ${err.body}`, true)
|
||||
}
|
||||
|
||||
@@ -12,8 +12,9 @@
|
||||
import type { SessionRuntime } from './sessionRuntime.svelte'
|
||||
import { Loader2 } from 'lucide-svelte'
|
||||
import {
|
||||
entityEditorHref,
|
||||
entityListHref,
|
||||
entityListPage,
|
||||
pageHref,
|
||||
resolvePreviewTab,
|
||||
parsePreviewItemRoute,
|
||||
parsePreviewSelectedId,
|
||||
@@ -87,7 +88,11 @@
|
||||
|
||||
// Bumped to remount a hosted entity editor, which is how it re-reads the item
|
||||
// from the server: its own state is built at mount from the draft cell, so
|
||||
// there is nothing to refresh in place once that cell has been dropped.
|
||||
// there is nothing to refresh in place once that cell has been dropped. The
|
||||
// acting workspace joins it in the key for the same reason — these editors
|
||||
// load and save against the workspace they were mounted with, so a session
|
||||
// that rescopes (a staged fork materialising on first send) must not leave
|
||||
// them bound to the previous one.
|
||||
let entityNonce = $state(0)
|
||||
|
||||
// Pages whose theme we mirror on live toggles. Regular apps are the only item
|
||||
@@ -167,14 +172,27 @@
|
||||
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: pageHref(page.path),
|
||||
href: entityListHref(whereIs(tab)),
|
||||
label: page.label
|
||||
})
|
||||
}
|
||||
: undefined
|
||||
)
|
||||
|
||||
// Follow a rename: the editor stays mounted and keeps editing the item, but the
|
||||
// tab, its label, the chat's ACTIVE PREVIEW and the draft key all address it by
|
||||
// path — so they have to move with it, or they name an item that no longer exists.
|
||||
const retargetTo = $derived(
|
||||
slot.kind === 'entity' && runtime
|
||||
? (newPath: string) =>
|
||||
runtime.previewTabs.retargetTabTo(tab.id, entityEditorHref(whereIs(tab), newPath))
|
||||
: 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
|
||||
@@ -354,7 +372,7 @@
|
||||
<!-- Dynamic import for the same reason as the editors above: these pull in
|
||||
the runnable pickers and the resource-type schema forms. Keyed on the
|
||||
refresh nonce so a dropped draft cell remounts the editor (see reload). -->
|
||||
{#key entityNonce}
|
||||
{#key `${workspaceId}#${entityNonce}`}
|
||||
{#if slot.entityKind === 'trigger_schedule'}
|
||||
{#await import('./ScheduleEditorView.svelte')}
|
||||
{@render editorLoading()}
|
||||
@@ -365,13 +383,23 @@
|
||||
{#await import('./ResourceEditorView.svelte')}
|
||||
{@render editorLoading()}
|
||||
{:then Module}
|
||||
<Module.default path={slot.path} {workspaceId} onBack={backToList} />
|
||||
<Module.default
|
||||
path={slot.path}
|
||||
{workspaceId}
|
||||
onBack={backToList}
|
||||
onRenamed={retargetTo}
|
||||
/>
|
||||
{/await}
|
||||
{:else}
|
||||
{:else if slot.entityKind === 'variable'}
|
||||
{#await import('./VariableEditorView.svelte')}
|
||||
{@render editorLoading()}
|
||||
{:then Module}
|
||||
<Module.default path={slot.path} {workspaceId} onBack={backToList} />
|
||||
<Module.default
|
||||
path={slot.path}
|
||||
{workspaceId}
|
||||
onBack={backToList}
|
||||
onRenamed={retargetTo}
|
||||
/>
|
||||
{/await}
|
||||
{/if}
|
||||
{/key}
|
||||
|
||||
@@ -5,7 +5,8 @@
|
||||
let {
|
||||
path,
|
||||
workspaceId,
|
||||
onBack
|
||||
onBack,
|
||||
onRenamed
|
||||
}: {
|
||||
/** The resource this tab edits (the row its location deep-links). */
|
||||
path: string
|
||||
@@ -14,6 +15,10 @@
|
||||
workspaceId: string
|
||||
/** Back to the list this editor was reached through; the tab replaced it. */
|
||||
onBack?: () => void
|
||||
/** The item was saved under a different path. The tab addresses it by path —
|
||||
* as do its label, the chat's ACTIVE PREVIEW and the draft key — so the tab
|
||||
* has to follow, or all four keep naming an item that no longer exists. */
|
||||
onRenamed?: (newPath: string) => void
|
||||
} = $props()
|
||||
|
||||
let editor = $state<ResourceEditorDrawer | undefined>()
|
||||
@@ -28,4 +33,12 @@
|
||||
})
|
||||
</script>
|
||||
|
||||
<ResourceEditorDrawer bind:this={editor} useDrawer={false} workspace={workspaceId} {onBack} />
|
||||
<ResourceEditorDrawer
|
||||
bind:this={editor}
|
||||
useDrawer={false}
|
||||
workspace={workspaceId}
|
||||
{onBack}
|
||||
onSaved={(saved) => {
|
||||
if (saved && saved !== path) onRenamed?.(saved)
|
||||
}}
|
||||
/>
|
||||
|
||||
@@ -24,6 +24,12 @@
|
||||
|
||||
let editor = $state<ScheduleEditorInner | undefined>()
|
||||
|
||||
// A save leaves the mounted editor holding the pre-save config as its deployed
|
||||
// baseline — the drawer hides that by closing, which inline is a no-op, so the
|
||||
// banner would go on claiming unsaved changes and Discard would restore the
|
||||
// value the save just replaced. Remounting re-reads the saved schedule.
|
||||
let generation = $state(0)
|
||||
|
||||
// Load whenever the tab is pointed at another schedule; the component keeps
|
||||
// its identity across that, as it does for the drawer's row-to-row switch.
|
||||
// `isFlow` is a first guess only — loadScheduleCfg sets it from the loaded
|
||||
@@ -31,6 +37,9 @@
|
||||
$effect(() => {
|
||||
const p = path
|
||||
const e = editor
|
||||
// `generation` is tracked so a remount re-opens: `editor` is rebound to the
|
||||
// fresh instance, but reading it alone would not say the instance changed.
|
||||
generation
|
||||
if (!p || !e) return
|
||||
untrack(() => void e.openEdit(p, false))
|
||||
})
|
||||
@@ -40,21 +49,33 @@
|
||||
<!-- useDrawer=false renders the editor as a Section, the same inline form the
|
||||
script/flow trigger panel mounts (see SchedulePanel). `customLabel` is that
|
||||
Section's header, which is where the way back belongs. -->
|
||||
<ScheduleEditorInner bind:this={editor} useDrawer={false} allowDraft showDraftBanner>
|
||||
{#snippet customLabel()}
|
||||
<div class="flex flex-row items-center gap-2 min-w-0">
|
||||
{#if onBack}
|
||||
<Button
|
||||
variant="subtle"
|
||||
unifiedSize="sm"
|
||||
startIcon={{ icon: ArrowLeft }}
|
||||
on:click={onBack}
|
||||
title="Back to Schedules"
|
||||
iconOnly
|
||||
/>
|
||||
{/if}
|
||||
<span class="text-sm font-semibold truncate">Schedule</span>
|
||||
</div>
|
||||
{/snippet}
|
||||
</ScheduleEditorInner>
|
||||
{#key generation}
|
||||
<!-- No `allowDraft`: that switches the toolbar to the trigger-panel branch,
|
||||
whose deploy button is gated on the `trigger`/`isDeployed` a script or
|
||||
flow editor supplies for a trigger staged next to it. A standalone tab
|
||||
has neither, which leaves the schedule permanently unsavable — its
|
||||
situation is the drawer's, whose plain Save this restores. -->
|
||||
<ScheduleEditorInner
|
||||
bind:this={editor}
|
||||
useDrawer={false}
|
||||
showDraftBanner
|
||||
onUpdate={() => generation++}
|
||||
>
|
||||
{#snippet customLabel()}
|
||||
<div class="flex flex-row items-center gap-2 min-w-0">
|
||||
{#if onBack}
|
||||
<Button
|
||||
variant="subtle"
|
||||
unifiedSize="sm"
|
||||
startIcon={{ icon: ArrowLeft }}
|
||||
on:click={onBack}
|
||||
title="Back to Schedules"
|
||||
iconOnly
|
||||
/>
|
||||
{/if}
|
||||
<span class="text-sm font-semibold truncate">Schedule</span>
|
||||
</div>
|
||||
{/snippet}
|
||||
</ScheduleEditorInner>
|
||||
{/key}
|
||||
</div>
|
||||
|
||||
@@ -5,7 +5,8 @@
|
||||
let {
|
||||
path,
|
||||
workspaceId,
|
||||
onBack
|
||||
onBack,
|
||||
onRenamed
|
||||
}: {
|
||||
/** The variable this tab edits (the row its location deep-links). */
|
||||
path: string
|
||||
@@ -14,6 +15,10 @@
|
||||
workspaceId: string
|
||||
/** Back to the list this editor was reached through; the tab replaced it. */
|
||||
onBack?: () => void
|
||||
/** The item was saved under a different path. The tab addresses it by path —
|
||||
* as do its label, the chat's ACTIVE PREVIEW and the draft key — so the tab
|
||||
* has to follow, or all four keep naming an item that no longer exists. */
|
||||
onRenamed?: (newPath: string) => void
|
||||
} = $props()
|
||||
|
||||
let editor = $state<VariableEditor | undefined>()
|
||||
@@ -28,4 +33,12 @@
|
||||
})
|
||||
</script>
|
||||
|
||||
<VariableEditor bind:this={editor} useDrawer={false} workspace={workspaceId} {onBack} />
|
||||
<VariableEditor
|
||||
bind:this={editor}
|
||||
useDrawer={false}
|
||||
workspace={workspaceId}
|
||||
{onBack}
|
||||
onSaved={(saved) => {
|
||||
if (saved && saved !== path) onRenamed?.(saved)
|
||||
}}
|
||||
/>
|
||||
|
||||
@@ -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<typeof UserDraftDbSyncer.flush>[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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -120,13 +120,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). An entity-editor tab does
|
||||
// match — its location is a list page with the row in the hash, which the path
|
||||
// comparison drops — but it is in-realm and self-syncing too, so the host's
|
||||
// `reload` ignores it. 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<string>
|
||||
|
||||
@@ -12,7 +12,9 @@ import {
|
||||
parsePreviewSelectedId,
|
||||
previewLocationContext,
|
||||
previewLocationLabel,
|
||||
resolvePreviewTab
|
||||
resolvePreviewTab,
|
||||
entityListHref,
|
||||
entityEditorHref
|
||||
} from './previewRouter'
|
||||
|
||||
describe('drawerAnchorFor', () => {
|
||||
@@ -417,3 +419,22 @@ describe('artifact route', () => {
|
||||
expect(previewLocationLabel('artifact:abc')).toBe('Artifact')
|
||||
})
|
||||
})
|
||||
|
||||
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')
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import {
|
||||
ASSETS_PATH,
|
||||
AUDIT_LOGS_PATH,
|
||||
drawerHashFor,
|
||||
FOLDERS_PATH,
|
||||
GROUPS_PATH,
|
||||
pageKey,
|
||||
@@ -133,6 +134,21 @@ const IN_REALM_ENTITY_PAGES: Partial<Record<string, EntityEditorKind>> = {
|
||||
[VARIABLES_PATH]: 'variable'
|
||||
}
|
||||
|
||||
/** 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. */
|
||||
|
||||
@@ -352,6 +352,19 @@ describe('SessionPreviewTabs.open', () => {
|
||||
// A legacy app owns its own hash (the editor reads it as `context.hash`), so the
|
||||
// observer records app state into `loc`. Reading that as a drawer anchor would
|
||||
// retarget on reopen, and a same-document retarget forces a reload that discards
|
||||
// the state the user was looking at.
|
||||
it('focuses a legacy app whose own hash changed instead of reloading it', () => {
|
||||
const o = owner()
|
||||
const app = () => ({ type: 'page' as const, href: '/apps/edit/u/me/dash', label: 'dash' })
|
||||
o.open(app())
|
||||
const id = o.tabs[0].id
|
||||
o.observeLocation(id, '/apps/edit/u/me/dash#tab=2')
|
||||
|
||||
expect(o.open(app()).status).toBe('focused')
|
||||
expect(o.tabs).toHaveLength(1)
|
||||
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.
|
||||
@@ -380,19 +393,6 @@ describe('SessionPreviewTabs.open', () => {
|
||||
expect(o.tabs[0].url).toBe('/kafka_triggers')
|
||||
})
|
||||
|
||||
// the state the user was looking at.
|
||||
it('focuses a legacy app whose own hash changed instead of reloading it', () => {
|
||||
const o = owner()
|
||||
const app = () => ({ type: 'page' as const, href: '/apps/edit/u/me/dash', label: 'dash' })
|
||||
o.open(app())
|
||||
const id = o.tabs[0].id
|
||||
o.observeLocation(id, '/apps/edit/u/me/dash#tab=2')
|
||||
|
||||
expect(o.open(app()).status).toBe('focused')
|
||||
expect(o.tabs).toHaveLength(1)
|
||||
expect(o.tabs[0].url).toBe('/apps/edit/u/me/dash')
|
||||
})
|
||||
|
||||
// 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', () => {
|
||||
|
||||
@@ -61,8 +61,7 @@
|
||||
matchPreviewPage,
|
||||
pageKey,
|
||||
parseArtifactRoute,
|
||||
entityListPage,
|
||||
pageHref,
|
||||
entityListHref,
|
||||
parseEntityEditorRoute,
|
||||
parsePreviewItemRoute,
|
||||
previewLocationLabel,
|
||||
@@ -593,9 +592,9 @@
|
||||
: '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') {
|
||||
const page = entityListPage(entity.entityKind)
|
||||
if (page) owner.retargetTabTo(tab.id, pageHref(page.path))
|
||||
owner.retargetTabTo(tab.id, entityListHref(whereIs(tab)))
|
||||
} else if (effect === 'refresh' && mountedTabKeys.has(key)) {
|
||||
tabHosts[key]?.reload({ entity: 'refresh' })
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user