From 8449006e5975f485cfc47523422c1168eb78e087 Mon Sep 17 00:00:00 2001 From: "Tommy D. Rossi" Date: Sat, 24 Jan 2026 15:48:52 +0100 Subject: [PATCH] refactor: simplify screen recording code - Fix vite output path (was broken lib/background.mjs, now background.js) - Import recording types from protocol.ts instead of duplicating in background.ts - Remove dead getSessionId() and unused sessionId params from screen-recording.ts - Replace 4 executor wrapper functions with single withRecordingDefaults helper - Fix binary chunk routing with lastRecordingMetadataTabId tracking - Add ExtensionStopRecordingResult type for proper extension/relay separation Saves ~56 lines while improving correctness of multi-tab recording. --- extension/src/background.ts | 30 +++------------ extension/vite.config.mts | 7 +--- playwriter/src/cdp-relay.ts | 27 ++++++++++--- playwriter/src/executor.ts | 44 ++++++--------------- playwriter/src/protocol.ts | 11 ++++++ playwriter/src/screen-recording.ts | 61 ++++++++---------------------- 6 files changed, 64 insertions(+), 116 deletions(-) diff --git a/extension/src/background.ts b/extension/src/background.ts index 59a2748..6e8a41a 100644 --- a/extension/src/background.ts +++ b/extension/src/background.ts @@ -3,7 +3,7 @@ declare const process: { env: { PLAYWRITER_PORT: string } } import { createStore } from 'zustand/vanilla' import type { ExtensionState, ConnectionState, TabState, TabInfo, RecordingInfo } from './types' import type { CDPEvent, Protocol } from 'playwriter/src/cdp-types' -import type { ExtensionCommandMessage, ExtensionResponseMessage } from 'playwriter/src/protocol' +import type { ExtensionCommandMessage, ExtensionResponseMessage, StartRecordingParams, StopRecordingParams, IsRecordingParams, CancelRecordingParams, StartRecordingResult, ExtensionStopRecordingResult, IsRecordingResult, CancelRecordingResult } from 'playwriter/src/protocol' const RELAY_PORT = process.env.PLAYWRITER_PORT const RELAY_URL = `ws://127.0.0.1:${RELAY_PORT}/extension` @@ -1089,15 +1089,7 @@ function resolveTabIdFromSessionId(sessionId?: string): number | undefined { return found?.tabId } -interface StartRecordingParams { - sessionId?: string - frameRate?: number - audio?: boolean - videoBitsPerSecond?: number - audioBitsPerSecond?: number -} - -async function handleStartRecording(params: StartRecordingParams): Promise<{ success: true; tabId: number; startedAt: number } | { success: false; error: string }> { +async function handleStartRecording(params: StartRecordingParams): Promise { const tabId = resolveTabIdFromSessionId(params.sessionId) if (!tabId) { return { success: false, error: 'No connected tab found for recording. Click the Playwriter extension icon on the tab you want to record.' } @@ -1177,11 +1169,7 @@ async function handleStartRecording(params: StartRecordingParams): Promise<{ suc } } -interface StopRecordingParams { - sessionId?: string -} - -async function handleStopRecording(params: StopRecordingParams): Promise<{ success: true; tabId: number; duration: number } | { success: false; error: string }> { +async function handleStopRecording(params: StopRecordingParams): Promise { const tabId = resolveTabIdFromSessionId(params.sessionId) if (!tabId) { return { success: false, error: 'No connected tab found' } @@ -1225,11 +1213,7 @@ async function handleStopRecording(params: StopRecordingParams): Promise<{ succe } } -interface IsRecordingParams { - sessionId?: string -} - -async function handleIsRecording(params: IsRecordingParams): Promise<{ isRecording: boolean; tabId?: number; startedAt?: number }> { +async function handleIsRecording(params: IsRecordingParams): Promise { const tabId = resolveTabIdFromSessionId(params.sessionId) if (!tabId) { return { isRecording: false } @@ -1257,11 +1241,7 @@ async function handleIsRecording(params: IsRecordingParams): Promise<{ isRecordi } } -interface CancelRecordingParams { - sessionId?: string -} - -async function handleCancelRecording(params: CancelRecordingParams): Promise<{ success: boolean; error?: string }> { +async function handleCancelRecording(params: CancelRecordingParams): Promise { const tabId = resolveTabIdFromSessionId(params.sessionId) if (!tabId) { return { success: false, error: 'No connected tab found' } diff --git a/extension/vite.config.mts b/extension/vite.config.mts index b7cf115..9ca83d6 100644 --- a/extension/vite.config.mts +++ b/extension/vite.config.mts @@ -58,12 +58,7 @@ export default defineConfig({ welcome: resolve(__dirname, 'src/welcome.html'), }, output: { - entryFileNames: (chunkInfo) => { - if (chunkInfo.name === 'background') { - return 'lib/background.mjs'; - } - return '[name].js'; - }, + entryFileNames: '[name].js', format: 'es', }, }, diff --git a/playwriter/src/cdp-relay.ts b/playwriter/src/cdp-relay.ts index e331e71..392363e 100644 --- a/playwriter/src/cdp-relay.ts +++ b/playwriter/src/cdp-relay.ts @@ -100,6 +100,8 @@ export async function startPlayWriterCDPRelayServer({ resolveStop?: (result: StopRecordingResult) => void } const activeRecordings = new Map() + // Track which tabId just sent recordingData metadata - used to route the next binary chunk + let lastRecordingMetadataTabId: number | null = null const playwrightClients = new Map() let extensionWs: WSContext | null = null @@ -839,12 +841,20 @@ export async function startPlayWriterCDPRelayServer({ // Handle binary data (recording chunks) if (event.data instanceof ArrayBuffer || Buffer.isBuffer(event.data)) { const buffer = Buffer.isBuffer(event.data) ? event.data : Buffer.from(event.data) - // Find the recording that's waiting for data (the one we just received metadata for) - // The extension sends metadata first, then binary - for (const recording of activeRecordings.values()) { - recording.chunks.push(buffer) - logger?.log(pc.blue(`Received recording chunk for tab ${recording.tabId}: ${buffer.length} bytes (total chunks: ${recording.chunks.length})`)) - break // Only add to the first active recording + // Route to the recording that just sent metadata + const tabId = lastRecordingMetadataTabId + lastRecordingMetadataTabId = null + + if (tabId !== null) { + const recording = activeRecordings.get(tabId) + if (recording) { + recording.chunks.push(buffer) + logger?.log(pc.blue(`Received recording chunk for tab ${tabId}: ${buffer.length} bytes (total chunks: ${recording.chunks.length})`)) + } else { + logger?.log(pc.yellow(`Received recording chunk for unknown tab ${tabId}, ignoring`)) + } + } else { + logger?.log(pc.yellow('Received recording chunk without preceding metadata, ignoring')) } return } @@ -883,6 +893,11 @@ export async function startPlayWriterCDPRelayServer({ const { tabId, final } = (message as any).params const recording = activeRecordings.get(tabId) + // Track which tab sent this metadata for routing the next binary chunk + if (!final) { + lastRecordingMetadataTabId = tabId + } + if (recording && final) { // This is the final marker - write all chunks to file try { diff --git a/playwriter/src/executor.ts b/playwriter/src/executor.ts index 45f4249..469119b 100644 --- a/playwriter/src/executor.ts +++ b/playwriter/src/executor.ts @@ -591,36 +591,14 @@ export class PlaywrightExecutor { } // Screen recording functions (via chrome.tabCapture in extension - survives navigation) + // Recording uses chrome.tabCapture which requires activeTab permission. + // This permission is granted when the user clicks the Playwriter extension icon on a tab. const relayPort = this.cdpConfig.port || 19988 - - const startRecordingFn = async (options: { - page?: Page - frameRate?: number - videoBitsPerSecond?: number - audioBitsPerSecond?: number - audio?: boolean - outputPath: string - }) => { - // Recording uses chrome.tabCapture which requires activeTab permission. - // This permission is granted when the user clicks the Playwriter extension icon on a tab. - // Recording will work on any tab where the user has clicked the icon. - return startRecording({ - page: options.page || page, - relayPort, - ...options - }) - } - - const stopRecordingFn = async (options: { page?: Page } = {}) => { - return stopRecording({ page: options.page || page, relayPort }) - } - - const isRecordingFn = async (options: { page?: Page } = {}) => { - return isRecording({ page: options.page || page, relayPort }) - } - - const cancelRecordingFn = async (options: { page?: Page } = {}) => { - return cancelRecording({ page: options.page || page, relayPort }) + // Recording will work on any tab where the user has clicked the icon. + const withRecordingDefaults = ( + fn: (opts: T & { relayPort: number }) => Promise + ) => { + return (options: T = {} as T) => fn({ page: options.page || page, relayPort, ...options }) } const self = this @@ -642,10 +620,10 @@ export class PlaywrightExecutor { formatStylesAsText, getReactSource: getReactSourceFn, screenshotWithAccessibilityLabels: screenshotWithAccessibilityLabelsFn, - startRecording: startRecordingFn, - stopRecording: stopRecordingFn, - isRecording: isRecordingFn, - cancelRecording: cancelRecordingFn, + startRecording: withRecordingDefaults(startRecording), + stopRecording: withRecordingDefaults(stopRecording), + isRecording: withRecordingDefaults(isRecording), + cancelRecording: withRecordingDefaults(cancelRecording), resetPlaywright: async () => { const { page: newPage, context: newContext } = await self.reset() vmContextObj.page = newPage diff --git a/playwriter/src/protocol.ts b/playwriter/src/protocol.ts index f196bf2..e915f24 100644 --- a/playwriter/src/protocol.ts +++ b/playwriter/src/protocol.ts @@ -146,6 +146,17 @@ export type StartRecordingResult = { error: string } +/** Result from extension - doesn't include path/size since relay writes the file */ +export type ExtensionStopRecordingResult = { + success: true + tabId: number + duration: number +} | { + success: false + error: string +} + +/** Final result from relay - includes path/size after file is written */ export type StopRecordingResult = { success: true tabId: number diff --git a/playwriter/src/screen-recording.ts b/playwriter/src/screen-recording.ts index aa9accd..e5f5dc1 100644 --- a/playwriter/src/screen-recording.ts +++ b/playwriter/src/screen-recording.ts @@ -3,13 +3,13 @@ * Recording happens in the extension context, so it survives page navigation. * * This module communicates with the relay server which forwards commands to the extension. + * + * Note: Recording uses the first connected tab. Multi-tab recording support would require + * proper sessionId mapping between Playwright pages and extension tabs. */ import type { Page } from 'playwright-core' import type { - StartRecordingBody, - StopRecordingParams, - CancelRecordingParams, StartRecordingResult, StopRecordingResult, IsRecordingResult, @@ -17,7 +17,7 @@ import type { } from './protocol.js' export interface StartRecordingOptions { - /** Target page to record */ + /** Target page to record (currently unused - records first connected tab) */ page: Page /** Frame rate (default: 30) */ frameRate?: number @@ -34,7 +34,7 @@ export interface StartRecordingOptions { } export interface StopRecordingOptions { - /** Target page that is being recorded */ + /** Target page that is being recorded (currently unused) */ page: Page /** Relay server port (default: 19988) */ relayPort?: number @@ -46,20 +46,12 @@ export interface RecordingState { tabId?: number } -function getSessionId(page: Page): string | undefined { - // The page's _guid is Playwright-internal and doesn't match the extension's sessionId (pw-tab-X). - // For now, we don't pass sessionId and let the extension use the first connected tab. - // TODO: Add proper mapping between page and extension tab sessionIds - return undefined -} - /** * Start recording the page. * The recording is handled by the extension, so it survives page navigation. */ export async function startRecording(options: StartRecordingOptions): Promise { const { - page, frameRate = 30, videoBitsPerSecond = 2500000, audioBitsPerSecond = 128000, @@ -67,20 +59,11 @@ export async function startRecording(options: StartRecordingOptions): Promise { - const { page, relayPort = 19988 } = options - - const sessionId = getSessionId(page) + const { relayPort = 19988 } = options const response = await fetch(`http://127.0.0.1:${relayPort}/recording/stop`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ sessionId } satisfies StopRecordingParams), + body: JSON.stringify({}), }) const result = await response.json() as StopRecordingResult @@ -117,43 +98,31 @@ export async function stopRecording(options: StopRecordingOptions): Promise<{ pa throw new Error(`Failed to stop recording: ${result.error}`) } - return { - path: result.path, - duration: result.duration, - size: result.size, - } + return { path: result.path, duration: result.duration, size: result.size } } /** - * Check if recording is currently active on a page. + * Check if recording is currently active. */ export async function isRecording(options: { page: Page; relayPort?: number }): Promise { - const { page, relayPort = 19988 } = options + const { relayPort = 19988 } = options - const sessionId = getSessionId(page) - - const response = await fetch(`http://127.0.0.1:${relayPort}/recording/status?sessionId=${sessionId || ''}`) + const response = await fetch(`http://127.0.0.1:${relayPort}/recording/status`) const result = await response.json() as IsRecordingResult - return { - isRecording: result.isRecording, - startedAt: result.startedAt, - tabId: result.tabId, - } + return { isRecording: result.isRecording, startedAt: result.startedAt, tabId: result.tabId } } /** * Cancel recording without saving. */ export async function cancelRecording(options: { page: Page; relayPort?: number }): Promise { - const { page, relayPort = 19988 } = options - - const sessionId = getSessionId(page) + const { relayPort = 19988 } = options const response = await fetch(`http://127.0.0.1:${relayPort}/recording/cancel`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ sessionId } satisfies CancelRecordingParams), + body: JSON.stringify({}), }) const result = await response.json() as CancelRecordingResult