fix: point re-opened previews at the tab already showing them (#10538)

* fix: point re-opened previews at the tab already showing them

* fix: judge composed preview mutations as one change

* fix: treat a fullscreen preview as displayed when deciding to flash

frontend/src/lib/components/sessions/PreviewTabHost.svelte

@@ -143,8 +143,10 @@
 
 	let flashing = $state(false)
 	let flashTimer: ReturnType<typeof setTimeout> | 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}
 			<div class="p-4 text-sm text-tertiary">This artifact is no longer available.</div>
 		{/if}
-		<!-- Overlay: the source editor's opaque bg would cover a ring on the container. -->
-		<div
-			class="pointer-events-none absolute inset-0 z-30 ring-2 ring-inset ring-border-accent transition-opacity duration-300 {flashing
-				? 'opacity-100'
-				: 'opacity-0'}"
-			aria-hidden="true"
-		></div>
 	</div>
 {:else if mounted}
 	<iframe
@@ -265,3 +260,14 @@
 		class="absolute inset-0 w-full h-full border-0 bg-surface {visibility}"
 	></iframe>
 {/if}
+
+<!-- Flash ring, a sibling of every tab body rather than a child of one: an
+     editor's own opaque background (or an iframe's document) would paint over a
+     ring drawn inside it. -->
+<div
+	class="pointer-events-none absolute inset-0 z-30 ring-2 ring-inset ring-border-accent transition-opacity duration-300 {flashing &&
+	active
+		? 'opacity-100'
+		: 'opacity-0'}"
+	aria-hidden="true"
+></div>

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_<uuid>`).
 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<typeof setTimeout> | 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<T>(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<T>(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<T>(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)

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)
+	})
 })

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.`

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)
 		})
 	})