From 9f45acb0747918e121d12b1fb2234c7c560cb45a Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Mon, 6 Apr 2026 16:46:30 -0700 Subject: [PATCH] refactor: extract TabSession from BrowserManager for per-tab state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move per-tab state (refMap, lastSnapshot, frame) into a new TabSession class. BrowserManager delegates to the active TabSession via getActiveSession(). Zero behavior change — all existing tests pass. This is the foundation for the /batch endpoint: both /command and /batch will use the same handler functions with TabSession, eliminating shared state races during parallel tab execution. Co-Authored-By: Claude Opus 4.6 (1M context) --- browse/src/browser-manager.ts | 148 +++++++++++++++------------------- browse/src/tab-session.ts | 140 ++++++++++++++++++++++++++++++++ 2 files changed, 207 insertions(+), 81 deletions(-) create mode 100644 browse/src/tab-session.ts diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts index 3a7a106c..9e6bb210 100644 --- a/browse/src/browser-manager.ts +++ b/browse/src/browser-manager.ts @@ -18,12 +18,12 @@ import { chromium, type Browser, type BrowserContext, type BrowserContextOptions, type Page, type Locator, type Cookie } from 'playwright'; import { addConsoleEntry, addNetworkEntry, addDialogEntry, networkBuffer, type DialogEntry } from './buffers'; import { validateNavigationUrl } from './url-validation'; +import { TabSession, type RefEntry } from './tab-session'; -export interface RefEntry { - locator: Locator; - role: string; - name: string; -} +export type { RefEntry }; + +// Re-export TabSession for consumers +export { TabSession }; export interface BrowserState { cookies: Cookie[]; @@ -38,6 +38,7 @@ export class BrowserManager { private browser: Browser | null = null; private context: BrowserContext | null = null; private pages: Map = new Map(); + private tabSessions: Map = new Map(); private activeTabId: number = 0; private nextTabId: number = 1; private extraHeaders: Record = {}; @@ -46,14 +47,7 @@ export class BrowserManager { /** Server port — set after server starts, used by cookie-import-browser command */ public serverPort: number = 0; - // ─── Ref Map (snapshot → @e1, @e2, @c1, @c2, ...) ──────── - private refMap: Map = new Map(); - - // ─── Snapshot Diffing ───────────────────────────────────── - // NOT cleared on navigation — it's a text baseline for diffing - private lastSnapshot: string | null = null; - - // ─── Dialog Handling ────────────────────────────────────── + // ─── Dialog Handling (global, not per-tab) ────────────────── private dialogAutoAccept: boolean = true; private dialogPromptText: string | null = null; @@ -138,11 +132,11 @@ export class BrowserManager { * Get the ref map for external consumers (e.g., /refs endpoint). */ getRefMap(): Array<{ ref: string; role: string; name: string }> { - const refs: Array<{ ref: string; role: string; name: string }> = []; - for (const [ref, entry] of this.refMap) { - refs.push({ ref, role: entry.role, name: entry.name }); + try { + return this.getActiveSession().getRefEntries(); + } catch { + return []; } - return refs; } async launch() { @@ -216,7 +210,7 @@ export class BrowserManager { async launchHeaded(authToken?: string): Promise { // Clear old state before repopulating this.pages.clear(); - this.refMap.clear(); + this.tabSessions.clear(); this.nextTabId = 1; // Find the gstack extension directory for auto-loading @@ -430,6 +424,7 @@ export class BrowserManager { this.context.on('page', (page) => { const id = this.nextTabId++; this.pages.set(id, page); + this.tabSessions.set(id, new TabSession(page)); this.activeTabId = id; this.wirePageEvents(page); // Inject indicator on the new tab @@ -443,6 +438,7 @@ export class BrowserManager { const page = existingPages[0]; const id = this.nextTabId++; this.pages.set(id, page); + this.tabSessions.set(id, new TabSession(page)); this.activeTabId = id; this.wirePageEvents(page); // Inject indicator on restored page (addInitScript only fires on new navigations) @@ -517,6 +513,7 @@ export class BrowserManager { const page = await this.context.newPage(); const id = this.nextTabId++; this.pages.set(id, page); + this.tabSessions.set(id, new TabSession(page)); this.activeTabId = id; // Wire up console/network/dialog capture @@ -536,6 +533,7 @@ export class BrowserManager { await page.close(); this.pages.delete(tabId); + this.tabSessions.delete(tabId); // Switch to another tab if we closed the active one if (tabId === this.activeTabId) { @@ -550,9 +548,8 @@ export class BrowserManager { } switchTab(id: number, opts?: { bringToFront?: boolean }): void { - if (!this.pages.has(id)) throw new Error(`Tab ${id} not found`); + if (!this.tabSessions.has(id)) throw new Error(`Tab ${id} not found`); this.activeTabId = id; - this.activeFrame = null; // Frame context is per-tab // Only bring to front when explicitly requested (user-initiated tab switch). // Internal tab pinning (BROWSE_TAB) should NOT steal focus. if (opts?.bringToFront !== false) { @@ -582,7 +579,6 @@ export class BrowserManager { // Exact match — best case if (pageUrl === activeUrl && id !== this.activeTabId) { this.activeTabId = id; - this.activeFrame = null; return; } // Fuzzy match — origin+pathname (handles query param / fragment differences) @@ -599,7 +595,6 @@ export class BrowserManager { // Fall back to fuzzy match if (fuzzyId !== null) { this.activeTabId = fuzzyId; - this.activeFrame = null; } } @@ -624,11 +619,24 @@ export class BrowserManager { return tabs; } - // ─── Page Access ─────────────────────────────────────────── + // ─── Session Access ──────────────────────────────────────── + /** Get the TabSession for the active tab. */ + getActiveSession(): TabSession { + const session = this.tabSessions.get(this.activeTabId); + if (!session) throw new Error('No active page. Use "browse goto " first.'); + return session; + } + + /** Get a TabSession by tab ID. Used by /batch for parallel tab execution. */ + getSession(tabId: number): TabSession { + const session = this.tabSessions.get(tabId); + if (!session) throw new Error(`Tab ${tabId} not found`); + return session; + } + + // ─── Page Access (delegates to active session) ───────────── getPage(): Page { - const page = this.pages.get(this.activeTabId); - if (!page) throw new Error('No active page. Use "browse goto " first.'); - return page; + return this.getActiveSession().page; } getCurrentUrl(): string { @@ -639,60 +647,34 @@ export class BrowserManager { } } - // ─── Ref Map ────────────────────────────────────────────── + // ─── Ref Map (delegates to active session) ────────────────── setRefMap(refs: Map) { - this.refMap = refs; + this.getActiveSession().setRefMap(refs); } clearRefs() { - this.refMap.clear(); + this.getActiveSession().clearRefs(); } - /** - * Resolve a selector that may be a @ref (e.g., "@e3", "@c1") or a CSS selector. - * Returns { locator } for refs or { selector } for CSS selectors. - */ async resolveRef(selector: string): Promise<{ locator: Locator } | { selector: string }> { - if (selector.startsWith('@e') || selector.startsWith('@c')) { - const ref = selector.slice(1); // "e3" or "c1" - const entry = this.refMap.get(ref); - if (!entry) { - throw new Error( - `Ref ${selector} not found. Run 'snapshot' to get fresh refs.` - ); - } - const count = await entry.locator.count(); - if (count === 0) { - throw new Error( - `Ref ${selector} (${entry.role} "${entry.name}") is stale — element no longer exists. ` + - `Run 'snapshot' for fresh refs.` - ); - } - return { locator: entry.locator }; - } - return { selector }; + return this.getActiveSession().resolveRef(selector); } - /** Get the ARIA role for a ref selector, or null for CSS selectors / unknown refs. */ getRefRole(selector: string): string | null { - if (selector.startsWith('@e') || selector.startsWith('@c')) { - const entry = this.refMap.get(selector.slice(1)); - return entry?.role ?? null; - } - return null; + return this.getActiveSession().getRefRole(selector); } getRefCount(): number { - return this.refMap.size; + return this.getActiveSession().getRefCount(); } - // ─── Snapshot Diffing ───────────────────────────────────── + // ─── Snapshot Diffing (delegates to active session) ───────── setLastSnapshot(text: string | null) { - this.lastSnapshot = text; + this.getActiveSession().setLastSnapshot(text); } getLastSnapshot(): string | null { - return this.lastSnapshot; + return this.getActiveSession().getLastSnapshot(); } // ─── Dialog Control ─────────────────────────────────────── @@ -744,30 +726,20 @@ export class BrowserManager { await page.close().catch(() => {}); } this.pages.clear(); - this.clearRefs(); + this.tabSessions.clear(); } - // ─── Frame context ───────────────────────────────── - private activeFrame: import('playwright').Frame | null = null; - + // ─── Frame context (delegates to active session) ──────────── setFrame(frame: import('playwright').Frame | null): void { - this.activeFrame = frame; + this.getActiveSession().setFrame(frame); } getFrame(): import('playwright').Frame | null { - return this.activeFrame; + return this.getActiveSession().getFrame(); } - /** - * Returns the active frame if set, otherwise the current page. - * Use this for operations that work on both Page and Frame (locator, evaluate, etc.). - */ getActiveFrameOrPage(): import('playwright').Page | import('playwright').Frame { - // Auto-recover from detached frames (iframe removed/navigated) - if (this.activeFrame?.isDetached()) { - this.activeFrame = null; - } - return this.activeFrame ?? this.getPage(); + return this.getActiveSession().getActiveFrameOrPage(); } // ─── State Save/Restore (shared by recreateContext + handoff) ─ @@ -819,6 +791,7 @@ export class BrowserManager { const page = await this.context.newPage(); const id = this.nextTabId++; this.pages.set(id, page); + this.tabSessions.set(id, new TabSession(page)); this.wirePageEvents(page); if (saved.url) { @@ -886,6 +859,7 @@ export class BrowserManager { await page.close().catch(() => {}); } this.pages.clear(); + this.tabSessions.clear(); await this.context.close().catch(() => {}); // 3. Create new context with updated settings @@ -909,6 +883,7 @@ export class BrowserManager { // Fallback: create a clean context + blank tab try { this.pages.clear(); + this.tabSessions.clear(); if (this.context) await this.context.close().catch(() => {}); const contextOptions: BrowserContextOptions = { @@ -994,6 +969,7 @@ export class BrowserManager { this.context = newContext; this.browser = newContext.browser(); this.pages.clear(); + this.tabSessions.clear(); this.connectionMode = 'headed'; if (Object.keys(this.extraHeaders).length > 0) { @@ -1036,9 +1012,13 @@ export class BrowserManager { * The meta-command handler calls handleSnapshot() after this. */ resume(): void { - this.clearRefs(); + // Clear refs and frame on the active session + try { + const session = this.getActiveSession(); + session.clearRefs(); + session.setFrame(null); + } catch {} this.resetFailures(); - this.activeFrame = null; } getIsHeaded(): boolean { @@ -1063,11 +1043,12 @@ export class BrowserManager { // ─── Console/Network/Dialog/Ref Wiring ──────────────────── private wirePageEvents(page: Page) { - // Track tab close — remove from pages map, switch to another tab + // Track tab close — remove from pages and sessions maps, switch to another tab page.on('close', () => { for (const [id, p] of this.pages) { if (p === page) { this.pages.delete(id); + this.tabSessions.delete(id); console.log(`[browse] Tab closed (id=${id}, remaining=${this.pages.size})`); // If the closed tab was active, switch to another if (this.activeTabId === id) { @@ -1083,8 +1064,13 @@ export class BrowserManager { // (lastSnapshot is NOT cleared — it's a text baseline for diffing) page.on('framenavigated', (frame) => { if (frame === page.mainFrame()) { - this.clearRefs(); - this.activeFrame = null; // Navigation invalidates frame context + // Find the TabSession for this page and clear its per-tab state + for (const session of this.tabSessions.values()) { + if (session.page === page) { + session.onMainFrameNavigated(); + break; + } + } } }); diff --git a/browse/src/tab-session.ts b/browse/src/tab-session.ts new file mode 100644 index 00000000..e5e8279a --- /dev/null +++ b/browse/src/tab-session.ts @@ -0,0 +1,140 @@ +/** + * Per-tab session state. + * + * Extracted from BrowserManager to enable parallel tab execution in /batch. + * Each TabSession holds the state that is scoped to a single browser tab: + * page reference, element refs, snapshot baseline, and frame context. + * + * BrowserManager (global) + * └── tabSessions: Map + * ├── TabSession(page1) ← refMap, lastSnapshot, frame + * ├── TabSession(page2) ← refMap, lastSnapshot, frame + * └── TabSession(page3) ← refMap, lastSnapshot, frame + * + * The /command path gets the active session via bm.getActiveSession(). + * The /batch path gets specific sessions via bm.getSession(tabId). + * Both paths pass TabSession to the same handler functions. + */ + +import type { Page, Locator, Frame } from 'playwright'; + +export interface RefEntry { + locator: Locator; + role: string; + name: string; +} + +export class TabSession { + readonly page: Page; + + // ─── Ref Map (snapshot → @e1, @e2, @c1, @c2, ...) ──────── + private refMap: Map = new Map(); + + // ─── Snapshot Diffing ───────────────────────────────────── + // NOT cleared on navigation — it's a text baseline for diffing + private lastSnapshot: string | null = null; + + // ─── Frame context ───────────────────────────────────────── + private activeFrame: Frame | null = null; + + constructor(page: Page) { + this.page = page; + } + + // ─── Page Access ─────────────────────────────────────────── + getPage(): Page { + return this.page; + } + + // ─── Ref Map ────────────────────────────────────────────── + setRefMap(refs: Map) { + this.refMap = refs; + } + + clearRefs() { + this.refMap.clear(); + } + + /** + * Resolve a selector that may be a @ref (e.g., "@e3", "@c1") or a CSS selector. + * Returns { locator } for refs or { selector } for CSS selectors. + */ + async resolveRef(selector: string): Promise<{ locator: Locator } | { selector: string }> { + if (selector.startsWith('@e') || selector.startsWith('@c')) { + const ref = selector.slice(1); // "e3" or "c1" + const entry = this.refMap.get(ref); + if (!entry) { + throw new Error( + `Ref ${selector} not found. Run 'snapshot' to get fresh refs.` + ); + } + const count = await entry.locator.count(); + if (count === 0) { + throw new Error( + `Ref ${selector} (${entry.role} "${entry.name}") is stale — element no longer exists. ` + + `Run 'snapshot' for fresh refs.` + ); + } + return { locator: entry.locator }; + } + return { selector }; + } + + /** Get the ARIA role for a ref selector, or null for CSS selectors / unknown refs. */ + getRefRole(selector: string): string | null { + if (selector.startsWith('@e') || selector.startsWith('@c')) { + const entry = this.refMap.get(selector.slice(1)); + return entry?.role ?? null; + } + return null; + } + + getRefCount(): number { + return this.refMap.size; + } + + /** Get all ref entries for the /refs endpoint. */ + getRefEntries(): Array<{ ref: string; role: string; name: string }> { + return Array.from(this.refMap.entries()).map(([ref, entry]) => ({ + ref, role: entry.role, name: entry.name, + })); + } + + // ─── Snapshot Diffing ───────────────────────────────────── + setLastSnapshot(text: string | null) { + this.lastSnapshot = text; + } + + getLastSnapshot(): string | null { + return this.lastSnapshot; + } + + // ─── Frame context ───────────────────────────────────────── + setFrame(frame: Frame | null): void { + this.activeFrame = frame; + } + + getFrame(): Frame | null { + return this.activeFrame; + } + + /** + * Returns the active frame if set, otherwise the current page. + * Use this for operations that work on both Page and Frame (locator, evaluate, etc.). + */ + getActiveFrameOrPage(): Page | Frame { + // Auto-recover from detached frames (iframe removed/navigated) + if (this.activeFrame?.isDetached()) { + this.activeFrame = null; + } + return this.activeFrame ?? this.page; + } + + /** + * Called on main-frame navigation to clear stale refs and frame context. + */ + onMainFrameNavigated(): void { + this.clearRefs(); + this.activeFrame = null; + } +}