From afce1b24f848fc2b39c2b6046faf1019965daca8 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Sun, 7 Jun 2026 11:49:40 -0700 Subject: [PATCH] Address emulator PR review feedback Co-authored-by: Orca --- config/packaged-runtime-node-modules.cjs | 25 +++++- src/cli/specs/emulator.ts | 2 +- src/main/emulator/emulator-bridge.ts | 79 ++++--------------- .../emulator/emulator-session-registry.ts | 77 ++++++++++++++++++ src/main/runtime/orca-runtime.ts | 2 + .../tab-group/useTabGroupWorkspaceModel.ts | 6 +- src/shared/keybindings.ts | 6 +- 7 files changed, 125 insertions(+), 72 deletions(-) create mode 100644 src/main/emulator/emulator-session-registry.ts diff --git a/config/packaged-runtime-node-modules.cjs b/config/packaged-runtime-node-modules.cjs index b979be51313..244a3f49197 100644 --- a/config/packaged-runtime-node-modules.cjs +++ b/config/packaged-runtime-node-modules.cjs @@ -96,6 +96,21 @@ function readPackage(packageName, fromDir = projectDir) { } } +function isKnownOmittedServeSimDependency(packageName, fromDir) { + if (packageName !== 'inspect-webkit') { + return false + } + const serveSimPackageJsonPath = join(projectDir, 'node_modules', 'serve-sim', 'package.json') + if (!existsSync(serveSimPackageJsonPath)) { + return false + } + try { + return realpathSync(fromDir) === realpathSync(dirname(serveSimPackageJsonPath)) + } catch { + return false + } +} + function collectPackagedRuntimePackages() { const packages = new Map() const visit = (packageName, fromDir = projectDir) => { @@ -106,9 +121,13 @@ function collectPackagedRuntimePackages() { let packageInfo try { packageInfo = readPackage(packageName, fromDir) - } catch { - // Why: serve-sim lists inspect-webkit but some installs omit it until first CLI run; skip missing closure deps. - return + } catch (error) { + // Why: serve-sim declares inspect-webkit, but current installs omit it. + // Keep that escape hatch narrow so broken packages still fail packaging. + if (isKnownOmittedServeSimDependency(packageName, fromDir)) { + return + } + throw error } if (packages.has(packageInfo.name)) { return diff --git a/src/cli/specs/emulator.ts b/src/cli/specs/emulator.ts index 897d2566f6d..4a190658e34 100644 --- a/src/cli/specs/emulator.ts +++ b/src/cli/specs/emulator.ts @@ -26,7 +26,7 @@ export const EMULATOR_COMMAND_SPECS: CommandSpec[] = [ path: ['emulator', 'type'], summary: 'Type text (US ASCII only)', usage: 'orca emulator type [--device ] [--worktree ] [--json]', - allowedFlags: [...GLOBAL_FLAGS, 'text', 'device', 'emulator', 'worktree', 'text'], + allowedFlags: [...GLOBAL_FLAGS, 'text', 'device', 'emulator', 'worktree'], positionalArgs: ['text'] }, { diff --git a/src/main/emulator/emulator-bridge.ts b/src/main/emulator/emulator-bridge.ts index 87b2f454d2c..431580216ad 100644 --- a/src/main/emulator/emulator-bridge.ts +++ b/src/main/emulator/emulator-bridge.ts @@ -19,13 +19,13 @@ import { killServeSimHelperProcessesForDevice, listServeSimHelperProcessesForDevice } from './serve-sim-helper-processes' -import type { EmulatorBridgeOptions, EmulatorSessionState } from './emulator-bridge-types' +import type { EmulatorBridgeOptions } from './emulator-bridge-types' import { sendEmulatorGestureSequence, type EmulatorGesturePoint } from './emulator-gesture-sender' import { parseServeSimDetachedSession } from './serve-sim-detached-session' +import { EmulatorSessionRegistry } from './emulator-session-registry' export class EmulatorBridge { - private readonly activeByWorktree = new Map() - private readonly sessions = new Map() + private readonly sessionRegistry = new EmulatorSessionRegistry() private serveSimExecutable: ServeSimExecutable private readonly waitForEndpointReady: (endpoint: string) => Promise @@ -56,42 +56,15 @@ export class EmulatorBridge { info: EmulatorSessionInfo, options: { managed?: boolean } = {} ): void { - const key = info.deviceUdid - this.sessions.set(key, { - deviceUdid: info.deviceUdid, - wsUrl: info.wsUrl, - streamUrl: info.streamUrl, - axUrl: info.axUrl, - pid: info.helperPid, - managed: options.managed === true, - initialized: true - }) - this.activeByWorktree.set(worktreeId, key) + this.sessionRegistry.registerActive(worktreeId, info, options) } unregisterActiveEmulator(worktreeId: string): void { - this.activeByWorktree.delete(worktreeId) + this.sessionRegistry.unregisterWorktree(worktreeId) } getActiveForWorktree(worktreeId?: string): EmulatorSessionInfo | null { - if (!worktreeId) { - return null - } - const key = this.activeByWorktree.get(worktreeId) - if (!key) { - return null - } - const s = this.sessions.get(key) - if (!s) { - return null - } - return { - deviceUdid: s.deviceUdid, - wsUrl: s.wsUrl, - streamUrl: s.streamUrl, - axUrl: s.axUrl, - helperPid: s.pid - } + return this.sessionRegistry.getActiveForWorktree(worktreeId) } async getReusableActiveForWorktree( @@ -126,12 +99,12 @@ export class EmulatorBridge { worktreeId: string, options: { shutdownDevice?: boolean; managedOnly?: boolean } = {} ): Promise { - const key = this.activeByWorktree.get(worktreeId) + const key = this.sessionRegistry.getActiveSessionKey(worktreeId) if (!key) { return null } - const session = this.sessions.get(key) - this.activeByWorktree.delete(worktreeId) + const session = this.sessionRegistry.getSession(key) + this.sessionRegistry.unregisterWorktree(worktreeId) if (!session || (options.managedOnly && !session.managed)) { return null } @@ -142,12 +115,7 @@ export class EmulatorBridge { if (options.shutdownDevice) { await shutdownSimulatorDevice(session.deviceUdid).catch(() => {}) } - this.sessions.delete(key) - for (const [wt, activeKey] of this.activeByWorktree.entries()) { - if (activeKey === key) { - this.activeByWorktree.delete(wt) - } - } + this.sessionRegistry.clearSessionAndWorktrees(key) return session.deviceUdid } @@ -193,7 +161,7 @@ export class EmulatorBridge { } const target = this.getTargetOrThrow(opts) const udid = await this.ensureUdid(target.udid) - const session = this.sessions.get(udid) + const session = this.sessionRegistry.getSession(udid) if (!session?.wsUrl) { throw new EmulatorError('emulator_no_active', 'No active emulator stream for gesture input') } @@ -298,16 +266,10 @@ export class EmulatorBridge { : this.getTargetOrThrow({ worktreeId }) const udid = target.udid await this.stopServeSimForDevice(udid, { - helperPid: this.sessions.get(udid)?.pid, + helperPid: this.sessionRegistry.getSession(udid)?.pid, includeOrphaned: true }) - const key = udid - this.sessions.delete(key) - for (const [wt, k] of this.activeByWorktree.entries()) { - if (k === key) { - this.activeByWorktree.delete(wt) - } - } + this.sessionRegistry.clearSessionAndWorktrees(udid) return udid } @@ -317,23 +279,17 @@ export class EmulatorBridge { : this.getTargetOrThrow({ worktreeId }) const udid = target.udid await this.stopServeSimForDevice(udid, { - helperPid: this.sessions.get(udid)?.pid, + helperPid: this.sessionRegistry.getSession(udid)?.pid, includeOrphaned: true }) await shutdownSimulatorDevice(udid) - const key = udid - this.sessions.delete(key) - for (const [wt, k] of this.activeByWorktree.entries()) { - if (k === key) { - this.activeByWorktree.delete(wt) - } - } + this.sessionRegistry.clearSessionAndWorktrees(udid) return udid } async destroyAllSessions(): Promise { const promises: Promise[] = [] - for (const session of this.sessions.values()) { + for (const session of this.sessionRegistry.listSessions()) { if (session.managed) { promises.push( this.stopServeSimForDevice(session.deviceUdid, { helperPid: session.pid }) @@ -343,8 +299,7 @@ export class EmulatorBridge { } } await Promise.allSettled(promises) - this.sessions.clear() - this.activeByWorktree.clear() + this.sessionRegistry.clear() } async onAppQuit(): Promise { diff --git a/src/main/emulator/emulator-session-registry.ts b/src/main/emulator/emulator-session-registry.ts new file mode 100644 index 00000000000..f9a4217a84e --- /dev/null +++ b/src/main/emulator/emulator-session-registry.ts @@ -0,0 +1,77 @@ +import type { EmulatorSessionInfo } from './emulator-types' +import type { EmulatorSessionState } from './emulator-bridge-types' + +export class EmulatorSessionRegistry { + private readonly activeByWorktree = new Map() + private readonly sessions = new Map() + + registerActive( + worktreeId: string, + info: EmulatorSessionInfo, + options: { managed?: boolean } = {} + ): void { + const key = info.deviceUdid + this.sessions.set(key, { + deviceUdid: info.deviceUdid, + wsUrl: info.wsUrl, + streamUrl: info.streamUrl, + axUrl: info.axUrl, + pid: info.helperPid, + managed: options.managed === true, + initialized: true + }) + this.activeByWorktree.set(worktreeId, key) + } + + unregisterWorktree(worktreeId: string): void { + this.activeByWorktree.delete(worktreeId) + } + + getActiveForWorktree(worktreeId?: string): EmulatorSessionInfo | null { + if (!worktreeId) { + return null + } + const key = this.activeByWorktree.get(worktreeId) + if (!key) { + return null + } + const session = this.sessions.get(key) + return session ? toSessionInfo(session) : null + } + + getActiveSessionKey(worktreeId: string): string | null { + return this.activeByWorktree.get(worktreeId) ?? null + } + + getSession(key: string): EmulatorSessionState | undefined { + return this.sessions.get(key) + } + + listSessions(): EmulatorSessionState[] { + return [...this.sessions.values()] + } + + clearSessionAndWorktrees(key: string): void { + this.sessions.delete(key) + for (const [worktreeId, activeKey] of this.activeByWorktree.entries()) { + if (activeKey === key) { + this.activeByWorktree.delete(worktreeId) + } + } + } + + clear(): void { + this.sessions.clear() + this.activeByWorktree.clear() + } +} + +function toSessionInfo(session: EmulatorSessionState): EmulatorSessionInfo { + return { + deviceUdid: session.deviceUdid, + wsUrl: session.wsUrl, + streamUrl: session.streamUrl, + axUrl: session.axUrl, + helperPid: session.pid + } +} diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 814dcbdbd7a..024f4ba549a 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -12220,6 +12220,8 @@ export class OrcaRuntimeService { } private dropDisconnectedPtyRecord(ptyId: string): void { + // Why: pruning can remove a PTY without the normal exit callback. + serveSimStateWatcher.unbindPty(ptyId) this.ptysById.delete(ptyId) this.recentPtyOutputById.delete(ptyId) this.ptyOutputSequenceById.delete(ptyId) diff --git a/src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts b/src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts index 362ab44bec2..7b8067c94fd 100644 --- a/src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts +++ b/src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts @@ -574,11 +574,7 @@ export function useTabGroupWorkspaceModel({ newSimulatorTab: () => { // Why: mobile simulators are most useful beside the current tab group. // Re-open focuses the existing worktree simulator instead of spawning duplicates. - const wtId = useAppStore.getState().activeWorktreeId - if (!wtId) { - return - } - ensureSimulatorTab(wtId, { + ensureSimulatorTab(worktreeId, { placement: 'rightSplit', targetGroupId: groupId, surfacePane: true diff --git a/src/shared/keybindings.ts b/src/shared/keybindings.ts index c7082c1f8dc..0d0d12d360d 100644 --- a/src/shared/keybindings.ts +++ b/src/shared/keybindings.ts @@ -426,7 +426,11 @@ export const KEYBINDING_DEFINITIONS: readonly KeybindingDefinition[] = [ group: 'Tabs', scope: 'tabs', searchKeywords: ['shortcut', 'tab', 'simulator', 'emulator', 'mobile', 'ios', 'new'], - defaultBindings: platformBindings(['Mod+Shift+E']) + defaultBindings: { + darwin: ['Mod+Shift+E'], + linux: [], + win32: [] + } }, { id: 'tab.newMarkdown',