diff --git a/frontend/src/lib/components/sessions/PreviewTabHost.svelte b/frontend/src/lib/components/sessions/PreviewTabHost.svelte index 93559023bb..8cd91b456f 100644 --- a/frontend/src/lib/components/sessions/PreviewTabHost.svelte +++ b/frontend/src/lib/components/sessions/PreviewTabHost.svelte @@ -143,8 +143,10 @@ let flashing = $state(false) let flashTimer: ReturnType | undefined - // Guard against the effect's non-pulse reruns (tab/runtime changes) firing a flash. - let lastPulseNonce = -1 + // Guard against the effect's non-pulse reruns (tab/runtime changes) firing a + // flash. Seeded from the current nonce: a pulse from before this host mounted + // is moot, the tab appearing is itself the change the flash would point at. + let lastPulseNonce = runtime?.previewTabs.focusPulse.nonce ?? -1 $effect(() => { const pulse = runtime?.previewTabs.focusPulse if (!pulse || pulse.nonce === lastPulseNonce) return @@ -242,13 +244,6 @@ {:else if !runtime?.manager.artifacts.loading}
This artifact is no longer available.
{/if} - - {:else if mounted} {/if} + + + diff --git a/frontend/src/lib/components/sessions/sessionPreviewTabs.svelte.ts b/frontend/src/lib/components/sessions/sessionPreviewTabs.svelte.ts index c4fbf47a3f..c96f687650 100644 --- a/frontend/src/lib/components/sessions/sessionPreviewTabs.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionPreviewTabs.svelte.ts @@ -59,13 +59,17 @@ function targetUrl(target: PreviewTarget): string { // Point a tab at a new destination. Clears `friendlyLabel`/`friendlyPath` // (bound to the previous editor's item): a new editor re-stamps them, and // navigating to a plain page must drop the stale name so the tab falls back -// to the location label. +// to the location label. Only on an actual change of destination, though — +// nothing re-stamps a tab that stays on the item it already hosts, so wiping +// there would strand its label at the storage path (`…/draft_`). function retargetTab(tab: SessionPreviewTab, url: string): void { + if (tab.url !== url) { + tab.friendlyLabel = undefined + tab.friendlyPath = undefined + tab.editorNamed = undefined + } tab.url = url tab.loc = url - tab.friendlyLabel = undefined - tab.friendlyPath = undefined - tab.editorNamed = undefined } // Strip the query params the sessions preview injects into iframe URLs @@ -166,6 +170,13 @@ export class SessionPreviewTabs { // Ephemeral UI signals — not part of the persisted snapshot. #focusPulse = $state({ id: '', nonce: 0 }) #reloadPulse = $state({ id: '', nonce: 0 }) + // Set while a mutation sequence is being judged as a whole (see asOneChange). + #pulsing = false + // Fullscreen overrides the collapsed layout, so the panel can be on screen + // while `#collapsed` still says otherwise. Page-level state (it outlives a + // session switch, unlike the persisted per-session flag), pushed in here so + // the flash decision reads what the user can actually see. + #fullscreen = false readonly #adapter: PreviewTabsAdapter readonly #flushDelay: number #flushHandle: ReturnType | undefined @@ -228,10 +239,66 @@ export class SessionPreviewTabs { this.#schedulePersist() } + // Whether the panel is on screen at all — fullscreen wins over collapsed. + setFullscreen(fullscreen: boolean): void { + this.#fullscreen = fullscreen + } + + // The tab the user is actually looking at, or undefined when nothing is on screen. + #displayedTab(): SessionPreviewTab | undefined { + if (this.#collapsed && !this.#fullscreen) return undefined + return this.#tabs.find((t) => t.id === this.#activeId) + } + + // Run a caller's own multi-call sequence (select + navigate + reveal) as a + // single change for the flash decision. Judging each call separately would + // flash a tab the sequence had just switched to, since the step that made it + // visible is not the step that finds nothing left to change. `mutate` must be + // synchronous: the verdict is read the moment it returns. + asOneChange(mutate: () => T): T { + return this.#pulsingIfUnchanged(mutate) + } + + // Run a tab mutation, flashing the displayed tab's border when it left the + // panel showing exactly what it already showed. Re-opening a destination that + // is already on screen is otherwise indistinguishable from a dead click. + #pulsingIfUnchanged(mutate: () => T): T { + // Already inside a sequence: that outer call owns the decision, and an + // inner open()/navigate() must not rule on its own slice of it. + if (this.#pulsing) return mutate() + this.#pulsing = true + try { + return this.#pulseIfSameDestination(mutate) + } finally { + this.#pulsing = false + } + } + + #pulseIfSameDestination(mutate: () => T): T { + const before = this.#displayedTab() + const shown = before && { id: before.id, url: before.url, loc: before.loc } + const result = mutate() + const after = this.#displayedTab() + if ( + shown && + after && + after.id === shown.id && + after.url === shown.url && + after.loc === shown.loc + ) { + this.pulseFocus(after.id) + } + return result + } + // Open — or focus, if already shown — a tab for a destination, and reveal the // panel. An editable item dedupes against the tab already hosting that same // (kind, path); anything else dedupes on the tab's observed location. open(target: PreviewTarget): { status: 'opened' | 'focused' } { + return this.#pulsingIfUnchanged(() => this.#open(target)) + } + + #open(target: PreviewTarget): { status: 'opened' | 'focused' } { const editorTarget = editorTargetFor(target) // A fresh session starts collapsed, so without this the tab opens behind a // collapsed panel and the user sees nothing change. @@ -276,8 +343,12 @@ export class SessionPreviewTabs { } // Focus the tab currently *showing* this destination instead of opening a // duplicate. Matched on the observed `loc`, not `url`: a tab that was - // opened here but navigated away no longer counts as showing it. - const shown = this.#tabs.find((t) => t.loc === url) + // opened here but navigated away no longer counts as showing it. Both sides + // are canonicalized because a caller may bake `?workspace=` into the href + // (the frame re-injects it from the session anyway) while the observed loc + // has had it stripped — comparing raw would reopen the page as a duplicate. + const canonicalUrl = canonicalizeObservedLoc(url) + const shown = this.#tabs.find((t) => canonicalizeObservedLoc(t.loc) === canonicalUrl) if (shown) { this.#activeId = shown.id this.#flush() @@ -294,6 +365,10 @@ export class SessionPreviewTabs { // Re-point the active tab at a destination (breadcrumb pick / in-editor link / // iframe-posted editor navigation). navigate(target: PreviewTarget): void { + this.#pulsingIfUnchanged(() => this.#navigate(target)) + } + + #navigate(target: PreviewTarget): void { const t = this.#tabs.find((x) => x.id === this.#activeId) if (!t) return const editorTarget = editorTargetFor(target) diff --git a/frontend/src/lib/components/sessions/sessionPreviewTabs.test.ts b/frontend/src/lib/components/sessions/sessionPreviewTabs.test.ts index b8c1ba2f91..fa3b4ac11d 100644 --- a/frontend/src/lib/components/sessions/sessionPreviewTabs.test.ts +++ b/frontend/src/lib/components/sessions/sessionPreviewTabs.test.ts @@ -681,4 +681,95 @@ describe('SessionPreviewTabs.pulseFocus', () => { o.pulseFocus('tab-b') expect(o.focusPulse).toEqual({ id: 'tab-b', nonce: 3 }) }) + + it('fires on re-opening whatever the panel already displays, for any tab kind', () => { + const o = owner() + o.open(rawAppTarget) + expect(o.focusPulse.nonce).toBe(0) + o.open(rawAppTarget) + expect(o.focusPulse).toEqual({ id: o.activeId, nonce: 1 }) + // A second tab taking over is its own visible change. + o.open(pageTarget) + expect(o.focusPulse.nonce).toBe(1) + o.open(pageTarget) + expect(o.focusPulse).toEqual({ id: o.activeId, nonce: 2 }) + // Re-pointing the displayed tab elsewhere changes what is on screen. + o.navigate(scriptTarget) + expect(o.focusPulse.nonce).toBe(2) + o.navigate(scriptTarget) + expect(o.focusPulse.nonce).toBe(3) + }) + + it('still flashes a collapsed-but-fullscreen panel', () => { + const o = owner() + o.open(rawAppTarget) + o.setCollapsed(true) + // Fullscreen carries over from the previous session and overrides collapse, + // so the tab is on screen and a re-open of it changes nothing visible. + o.setFullscreen(true) + o.open(rawAppTarget) + expect(o.focusPulse.nonce).toBe(1) + }) + + it('judges a composed select+navigate as one change', () => { + const o = owner() + o.open(pageTarget) + const runs = o.tabs[0].id + o.open(rawAppTarget) + expect(o.focusPulse.nonce).toBe(0) + // open_page reusing a *background* page tab: the switch is the visible change. + o.asOneChange(() => { + o.select(runs) + o.navigate(pageTarget) + }) + expect(o.focusPulse.nonce).toBe(0) + // Same sequence once that tab is already displayed: nothing changes, so flash. + o.asOneChange(() => { + o.select(runs) + o.navigate(pageTarget) + }) + expect(o.focusPulse).toEqual({ id: runs, nonce: 1 }) + }) + + it('keeps the editor-stamped label when re-pointed at the same item', () => { + const o = owner() + o.open(scriptTarget) + o.setEditorFriendlyLabel({ kind: 'script', path: 'u/me/foo' }, 'My script', 'u/me/staged') + // Nothing re-stamps a tab that never changed item, so a wipe here would be + // permanent — and would make the "nothing changed" flash a lie. + o.navigate(scriptTarget) + expect(o.tabs[0].friendlyLabel).toBe('My script') + expect(o.tabs[0].friendlyPath).toBe('u/me/staged') + o.navigate(flowTarget) + expect(o.tabs[0].friendlyLabel).toBeUndefined() + }) + + it('focuses (and flashes) a run tab whose href carries ?workspace=', () => { + const o = owner() + const run: PreviewTarget = { + type: 'page', + href: `${base}/run/job-1?workspace=fork`, + label: 'Run' + } + o.open(run) + // What the frame reports back has the injected params stripped. + o.observeLocation(o.activeId, `${base}/run/job-1?workspace=fork&nomenubar=true`) + expect(o.tabs.length).toBe(1) + expect(o.open(run).status).toBe('focused') + expect(o.tabs.length).toBe(1) + expect(o.focusPulse.nonce).toBe(1) + }) + + it('stays quiet when the re-open is what reveals the tab', () => { + const o = owner() + o.open(rawAppTarget) + o.setCollapsed(true) + // Un-collapsing onto the same tab is already visible. + o.open(rawAppTarget) + expect(o.focusPulse.nonce).toBe(0) + // Switching back to a background tab is too. + o.open(pageTarget) + o.open(rawAppTarget) + expect(o.focusPulse.nonce).toBe(0) + }) }) diff --git a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts index 5de8fb88ac..aaf0173140 100644 --- a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts @@ -482,14 +482,7 @@ function createRuntime(session: Session): SessionRuntime { } manager.openArtifact = (id, name) => { - // Capture before open() un-collapses / re-activates: flash only when the tab - // was already the displayed one (nothing else visibly changes). - const wasDisplayed = !previewTabs.collapsed - const prevActive = previewTabs.activeId - const { status } = previewTabs.open({ type: 'artifact', id, name }) - if (status === 'focused' && wasDisplayed && previewTabs.activeId === prevActive) { - previewTabs.pulseFocus(previewTabs.activeId) - } + previewTabs.open({ type: 'artifact', id, name }) } manager.closeArtifact = (id) => previewTabs.closeArtifact(id) // Key the store before any configureGlobalMode runs, so a new session's first create shows at once. @@ -1054,9 +1047,13 @@ setOpenPagePreviewHandler(({ sessionId: callerSessionId, href, label, newTab }) // would silently not re-fire — force a load. Hashless targets need no // reload: focusing the already-correct view is enough. const unchanged = href.includes('#') && (existing.loc || existing.url) === href - owner.select(existing.id) - owner.navigate({ type: 'page', href, label }) - owner.setCollapsed(false) + // One change, not three: switching to a background tab is already visible, + // so the navigate that follows must not read as "nothing happened". + owner.asOneChange(() => { + owner.select(existing.id) + owner.navigate({ type: 'page', href, label }) + owner.setCollapsed(false) + }) if (unchanged) { owner.pulseReload(existing.id) return `Re-opened the ${label} preview tab on the requested view.` diff --git a/frontend/src/routes/(root)/(logged)/sessions/+page.svelte b/frontend/src/routes/(root)/(logged)/sessions/+page.svelte index acbfb13164..1abf9c2d18 100644 --- a/frontend/src/routes/(root)/(logged)/sessions/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/sessions/+page.svelte @@ -318,6 +318,12 @@ let emptyStateNewTabOpen = $state(false) let fullscreen = $state(false) + // Fullscreen is page state, not per-session, so it outlives a session switch — + // tell the incoming session's model, whose own collapsed flag it overrides, or + // re-opening the item plainly on screen would be judged invisible and not flash. + $effect(() => { + owner?.setFullscreen(fullscreen) + }) // Collapse the preview panel to give the chat the full width. Per-session and // owned by the runtime's previewTabs (restored on switch, written back on // toggle) so it survives session switches with the rest of the tab model. @@ -477,8 +483,7 @@ // (or focus, if already shown) the item's preview in the active session's panel — // the visible chat is always the active session, so `owner` is its panel. Read // `owner` lazily inside the handler (not in the effect body) so this registers - // once, not on every session switch. A 'focused' open leaves the tab where it is, - // so pulse it to make the click visibly land. + // once, not on every session switch. $effect(() => { return registerToolDisplayActionHandler('open_item_preview', (action) => { if (action.type !== 'open_item_preview') return @@ -486,8 +491,7 @@ if (!o) return const target = previewTargetForSessionTarget(action.previewKind, action.path) if (!target) return - const { status } = o.open(target) - if (status === 'focused') o.pulseFocus(o.activeId) + o.open(target) }) })