From f79160e5989c3a653f01df2ae20ec85966c9fea9 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Sat, 5 Sep 2026 19:24:05 +0200 Subject: [PATCH] fix(sessions): rebind hosted entity editors on rescope, rename and save Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KcyBRxE8j3ujCYkum9KrC4 --- .../components/ResourceEditorDrawer.svelte | 18 ++++-- .../src/lib/components/VariableEditor.svelte | 11 +++- .../components/sessions/PreviewTabHost.svelte | 42 +++++++++++--- .../sessions/ResourceEditorView.svelte | 17 +++++- .../sessions/ScheduleEditorView.svelte | 55 +++++++++++++------ .../sessions/VariableEditorView.svelte | 17 +++++- .../components/sessions/pageDrawerSession.ts | 6 +- .../lib/components/sessions/previewPaths.ts | 7 +++ .../lib/components/sessions/previewReload.ts | 13 ++--- .../components/sessions/previewRouter.test.ts | 23 +++++++- .../lib/components/sessions/previewRouter.ts | 16 ++++++ .../sessions/sessionPreviewTabs.test.ts | 26 ++++----- .../(root)/(logged)/sessions/+page.svelte | 7 +-- 13 files changed, 195 insertions(+), 63 deletions(-) diff --git a/frontend/src/lib/components/ResourceEditorDrawer.svelte b/frontend/src/lib/components/ResourceEditorDrawer.svelte index 4dc9cebea9..4939a1ef20 100644 --- a/frontend/src/lib/components/ResourceEditorDrawer.svelte +++ b/frontend/src/lib/components/ResourceEditorDrawer.svelte @@ -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} > diff --git a/frontend/src/lib/components/VariableEditor.svelte b/frontend/src/lib/components/VariableEditor.svelte index ca69385a40..8b0d78800b 100644 --- a/frontend/src/lib/components/VariableEditor.svelte +++ b/frontend/src/lib/components/VariableEditor.svelte @@ -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) } diff --git a/frontend/src/lib/components/sessions/PreviewTabHost.svelte b/frontend/src/lib/components/sessions/PreviewTabHost.svelte index a029b34caa..b0ae98ecf9 100644 --- a/frontend/src/lib/components/sessions/PreviewTabHost.svelte +++ b/frontend/src/lib/components/sessions/PreviewTabHost.svelte @@ -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 @@ - {#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} - + {/await} - {:else} + {:else if slot.entityKind === 'variable'} {#await import('./VariableEditorView.svelte')} {@render editorLoading()} {:then Module} - + {/await} {/if} {/key} diff --git a/frontend/src/lib/components/sessions/ResourceEditorView.svelte b/frontend/src/lib/components/sessions/ResourceEditorView.svelte index 7aeec5932e..12c5720766 100644 --- a/frontend/src/lib/components/sessions/ResourceEditorView.svelte +++ b/frontend/src/lib/components/sessions/ResourceEditorView.svelte @@ -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() @@ -28,4 +33,12 @@ }) - + { + if (saved && saved !== path) onRenamed?.(saved) + }} +/> diff --git a/frontend/src/lib/components/sessions/ScheduleEditorView.svelte b/frontend/src/lib/components/sessions/ScheduleEditorView.svelte index e70e205a06..f221d63ec0 100644 --- a/frontend/src/lib/components/sessions/ScheduleEditorView.svelte +++ b/frontend/src/lib/components/sessions/ScheduleEditorView.svelte @@ -24,6 +24,12 @@ let editor = $state() + // 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 @@ - - {#snippet customLabel()} -
- {#if onBack} -
- {/snippet} - + {#key generation} + + generation++} + > + {#snippet customLabel()} +
+ {#if onBack} +
+ {/snippet} + + {/key} diff --git a/frontend/src/lib/components/sessions/VariableEditorView.svelte b/frontend/src/lib/components/sessions/VariableEditorView.svelte index b2e9071abe..b6397cecc6 100644 --- a/frontend/src/lib/components/sessions/VariableEditorView.svelte +++ b/frontend/src/lib/components/sessions/VariableEditorView.svelte @@ -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() @@ -28,4 +33,12 @@ }) - + { + if (saved && saved !== path) onRenamed?.(saved) + }} +/> 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.ts b/frontend/src/lib/components/sessions/previewReload.ts index 1847f8d6cb..83be531bc4 100644 --- a/frontend/src/lib/components/sessions/previewReload.ts +++ b/frontend/src/lib/components/sessions/previewReload.ts @@ -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 diff --git a/frontend/src/lib/components/sessions/previewRouter.test.ts b/frontend/src/lib/components/sessions/previewRouter.test.ts index 9af1685b81..2720dadb43 100644 --- a/frontend/src/lib/components/sessions/previewRouter.test.ts +++ b/frontend/src/lib/components/sessions/previewRouter.test.ts @@ -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') + }) +}) diff --git a/frontend/src/lib/components/sessions/previewRouter.ts b/frontend/src/lib/components/sessions/previewRouter.ts index 581bcd6edf..2c7cbd8635 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, @@ -133,6 +134,21 @@ const IN_REALM_ENTITY_PAGES: Partial> = { [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. */ diff --git a/frontend/src/lib/components/sessions/sessionPreviewTabs.test.ts b/frontend/src/lib/components/sessions/sessionPreviewTabs.test.ts index c36a6f8445..83cf2b8a1e 100644 --- a/frontend/src/lib/components/sessions/sessionPreviewTabs.test.ts +++ b/frontend/src/lib/components/sessions/sessionPreviewTabs.test.ts @@ -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', () => { diff --git a/frontend/src/routes/(root)/(logged)/sessions/+page.svelte b/frontend/src/routes/(root)/(logged)/sessions/+page.svelte index 231ab4ca9e..ed070d6c21 100644 --- a/frontend/src/routes/(root)/(logged)/sessions/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/sessions/+page.svelte @@ -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' }) }