fix(desktop): harden preview session-scoping reuse paths
Review feedback on #87288 (Enough1122 + cross-vendor validation): - openPreview: reusing a pinned file row from another session keeps the owner's id (no id/sessionId split); the singleton Browser and artifacts reached from another session re-own to the navigating session so the new URL is not invisible in the session that opened it; the rail selection follows the row's final id instead of the pre-computed one. - preview-tile: merge the visible-list reveal/rehome listeners into a single rehome-then-reveal listener so ordering is explicit. - preview-strip-tools: the pin toggle reads the current store state at click time instead of the render-time closure. - Tests: pinned-reuse owner id, focused-tile-over-selection, and Browser re-own coverage — each mutation-verified (fails with the fix reverted). 42 targeted tests pass; renderer typecheck and eslint clean.
This commit is contained in:
committed by
brooklyn!
parent
b3e896246f
commit
824e107cb7
@@ -242,10 +242,15 @@ export function watchPreviewTiles(): void {
|
||||
}
|
||||
|
||||
$rightRailActiveTabId.listen(reveal)
|
||||
// Visible-list changes: a re-open of the already-active tab refreshes the
|
||||
// target through the visible list too, while tab mutations belonging to a
|
||||
// BACKGROUND session must not try to reveal panes that are not mounted.
|
||||
$visiblePreviewTabs.listen(reveal)
|
||||
// One listener for the visible list: re-home the selection FIRST (a session
|
||||
// switch or an unpin can leave the active tab outside the visible set, and
|
||||
// a stale reveal of a just-hidden tab is a no-op — its pane left the tree),
|
||||
// THEN reveal the tab the re-home left active. Registered after the mirror,
|
||||
// so the pane set is already synced when this runs.
|
||||
$visiblePreviewTabs.listen(() => {
|
||||
rehome()
|
||||
reveal()
|
||||
})
|
||||
|
||||
// A session switch (or an unpin) can leave the active tab outside the
|
||||
// visible set — re-home the selection to the first visible tab so the strip
|
||||
@@ -258,8 +263,6 @@ export function watchPreviewTiles(): void {
|
||||
}
|
||||
}
|
||||
|
||||
$visiblePreviewTabs.listen(rehome)
|
||||
|
||||
// And the reverse: clicking a preview TAB activates its pane in the TREE
|
||||
// only, so the store's selection must follow or `$previewTarget` (⌘L quote
|
||||
// labels, the titlebar's has-preview state) keeps reporting the previous
|
||||
|
||||
@@ -1,5 +1,8 @@
|
||||
import { afterEach, beforeEach, describe, expect, it } from 'vitest'
|
||||
|
||||
import { group } from '@/components/pane-shell/tree/model'
|
||||
import { $layoutTree, noteActiveTreeGroup } from '@/components/pane-shell/tree/store'
|
||||
|
||||
import { $rightRailActiveTabId, selectRightRailTab } from './layout'
|
||||
import {
|
||||
$browserPages,
|
||||
@@ -526,6 +529,79 @@ describe('preview session scoping', () => {
|
||||
expect($visiblePreviewTabs.get()).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('reusing an owned pinned row from another session keeps the owner id', () => {
|
||||
$selectedStoredSessionId.set('sess-1')
|
||||
openPreview(fileTarget('/work/a.html'))
|
||||
setPreviewTabPinned($previewTabs.get()[0]!.id, true)
|
||||
|
||||
$selectedStoredSessionId.set('sess-2')
|
||||
openPreview({ ...fileTarget('/work/a.html'), label: 'refreshed' })
|
||||
|
||||
expect($previewTabs.get()).toHaveLength(1)
|
||||
// The row keeps its OWNER's id — never an id/sessionId split (the id
|
||||
// would say sess-2 while the owner says sess-1 until the next unpin).
|
||||
expect($previewTabs.get()[0]).toMatchObject({
|
||||
id: 'file:sess-1:/work/a.html',
|
||||
sessionId: 'sess-1',
|
||||
pinned: true
|
||||
})
|
||||
expect($previewTabs.get()[0]?.target.label).toBe('refreshed')
|
||||
expect($rightRailActiveTabId.get()).toBe('file:sess-1:/work/a.html')
|
||||
})
|
||||
|
||||
it('follows the focused session tile over the sidebar selection', () => {
|
||||
$selectedStoredSessionId.set('sess-1')
|
||||
openPreview(fileTarget('/work/a.html'))
|
||||
$selectedStoredSessionId.set('sess-2')
|
||||
openPreview(fileTarget('/work/b.html'))
|
||||
|
||||
expect($visiblePreviewTabs.get().map(tab => tab.target.path)).toEqual(['/work/b.html'])
|
||||
|
||||
// A sess-1 session TILE is active in the interacted zone while sess-2 is
|
||||
// selected in the sidebar: the drawer belongs to the conversation being
|
||||
// typed in, so the visible set follows the FOCUSED session.
|
||||
const previousTree = $layoutTree.get()
|
||||
|
||||
try {
|
||||
$layoutTree.set(group(['session-tile:sess-1'], { active: 'session-tile:sess-1', id: 'grp-main' }))
|
||||
noteActiveTreeGroup('grp-main')
|
||||
|
||||
expect($visiblePreviewTabs.get().map(tab => tab.target.path)).toEqual(['/work/a.html'])
|
||||
|
||||
// Leaving the tile (workspace/route active) falls back to the selection.
|
||||
noteActiveTreeGroup(null)
|
||||
expect($visiblePreviewTabs.get().map(tab => tab.target.path)).toEqual(['/work/b.html'])
|
||||
} finally {
|
||||
// A failed assertion must not leak the tree/group state into the next
|
||||
// test — the focused derivation reads both.
|
||||
noteActiveTreeGroup(null)
|
||||
$layoutTree.set(previousTree)
|
||||
}
|
||||
})
|
||||
|
||||
it('re-owns the singleton Browser to the session that navigates it', () => {
|
||||
$selectedStoredSessionId.set('sess-1')
|
||||
openPreview(urlTarget('http://localhost:5174'))
|
||||
|
||||
// A second session navigating the Browser takes the surface with it:
|
||||
// keeping the original owner would leave the new URL invisible in the
|
||||
// session that just opened it.
|
||||
$selectedStoredSessionId.set('sess-2')
|
||||
openPreview(urlTarget('http://localhost:9999'))
|
||||
|
||||
expect($previewTabs.get()).toHaveLength(1)
|
||||
// The Browser id is minted (never reused), so assert the stable parts:
|
||||
// one surface, re-owned to the navigating session.
|
||||
expect($previewTabs.get()[0]?.id).toMatch(/^url:browser-/)
|
||||
expect($previewTabs.get()[0]).toMatchObject({ sessionId: 'sess-2' })
|
||||
expect($previewTabs.get()[0]?.target.url).toBe('http://localhost:9999')
|
||||
expect($visiblePreviewTabs.get()).toHaveLength(1)
|
||||
|
||||
// And sess-1 no longer sees the navigated surface — no context leak.
|
||||
$selectedStoredSessionId.set('sess-1')
|
||||
expect($visiblePreviewTabs.get()).toHaveLength(0)
|
||||
})
|
||||
|
||||
it('adoption dedupes against an already session-scoped row', () => {
|
||||
// Both rows for the same file coexist before the session appears.
|
||||
$previewTabs.set([
|
||||
|
||||
@@ -176,6 +176,7 @@ function parseTabList(parsed: unknown): PreviewTab[] {
|
||||
tab.target.kind === 'url' || tab.target.kind === 'file'
|
||||
? previewTabId(tab.target, tab.sessionId)
|
||||
: tab.id
|
||||
|
||||
deduped.set(id, { ...tab, id })
|
||||
}
|
||||
|
||||
@@ -400,6 +401,7 @@ $focusedStoredSessionId.listen(sessionId => {
|
||||
if (tabs.some(tab => tab.sessionId == null && !tab.pinned)) {
|
||||
const activeId = $rightRailActiveTabId.get()
|
||||
let newActiveId = activeId
|
||||
|
||||
const updated = tabs.map(tab => {
|
||||
if (tab.sessionId != null || tab.pinned) {
|
||||
return tab
|
||||
@@ -750,24 +752,48 @@ export function openPreview(target: PreviewTarget) {
|
||||
const sessionId = $focusedStoredSessionId.get() ?? undefined
|
||||
const id = target.kind === 'url' ? browserTabId(current) : previewTabId(target, sessionId)
|
||||
const index = current.findIndex(tab => tab.id === id)
|
||||
|
||||
const existing =
|
||||
index === -1 && target.kind === 'file'
|
||||
? current.find(
|
||||
tab => tab.pinned && tab.target.kind === 'file' && canonicalFileKey(tab.target) === canonicalFileKey(target)
|
||||
)
|
||||
: current[index]
|
||||
|
||||
// Reuse semantics depend on WHY the row was found:
|
||||
// - Same-session re-open (any kind): keep the owner and pin state — only
|
||||
// the target refreshes.
|
||||
// - A PINNED file row reached from ANOTHER session: keep its owner (and
|
||||
// therefore its owner-keyed id) — the pin is the explicit cross-session
|
||||
// workspace contract, and rekeying to the opening session would leave
|
||||
// `id` and `sessionId` disagreeing until the next unpin (which returns
|
||||
// the tab to that owner anyway).
|
||||
// - The singleton Browser or an artifact reached from another session: the
|
||||
// surface NAVIGATES, so it re-owns to the opening session — otherwise the
|
||||
// new URL would stay invisible in the session that just opened it.
|
||||
// - Ownerless legacy rows: adopted — rekeyed and stamped — into the opening
|
||||
// session (see the pinned-legacy test).
|
||||
// The id is derived from the FINAL session id so the two can never split.
|
||||
const tabSessionId =
|
||||
existing?.sessionId == null || existing.pinned || existing.sessionId === sessionId
|
||||
? existing?.sessionId ?? sessionId
|
||||
: sessionId
|
||||
|
||||
const tab: PreviewTab = {
|
||||
id,
|
||||
id: target.kind === 'url' ? id : previewTabId(target, tabSessionId),
|
||||
target: withRenderMode(target, existing?.target),
|
||||
sessionId: existing?.sessionId ?? sessionId,
|
||||
sessionId: tabSessionId,
|
||||
pinned: existing?.pinned
|
||||
}
|
||||
|
||||
const replaceIndex = existing ? current.indexOf(existing) : -1
|
||||
|
||||
$previewTabs.set(replaceIndex === -1 ? [...current, tab] : current.map((item, i) => (i === replaceIndex ? tab : item)))
|
||||
noteExplicitPreviewOpen(id)
|
||||
selectRightRailTab(id)
|
||||
noteExplicitPreviewOpen(tab.id)
|
||||
// Select the row's FINAL id: when a pinned row is reused from another
|
||||
// session, the id keeps the owner's session, so the pre-computed id would
|
||||
// point at nothing.
|
||||
selectRightRailTab(tab.id)
|
||||
}
|
||||
|
||||
const blankPage = (): PreviewTarget => ({ kind: 'url', label: 'Browser', source: 'about:blank', url: 'about:blank' })
|
||||
|
||||
Reference in New Issue
Block a user