src/renderer/src/components/emulator-pane/use-emulator-pane-session.ts (1)

115-115: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Infinite retry loop on attach failure (still present).

This is the same issue from the previous review: suppressAutoAttachRef.current = false at line 115 unconditionally resets the suppression flag. When attach() fails (lines 159-171), the catch block does not set suppressAutoAttachRef.current = true, so when loading returns to false in the finally block (line 173), the auto-attach effect (lines 233-238) re-fires and calls attach() again, creating an infinite retry loop.

Fix by moving line 115 to line 156 (after successful attach) or setting suppressAutoAttachRef.current = true in the catch block.

🔒 Proposed fix

Option 1: Only reset flag on success

   async (deviceTarget?: string) => {
     if (loading) {
       return
     }
-    suppressAutoAttachRef.current = false
     setLoading(true)
     setError(null)
     if (tabId) {
       useAppStore.getState().setTabLabel(tabId, 'Starting…')
     }
     let requestedTarget: string | undefined
     try {
       // ... attach logic ...
       applySession(res?.info, attached, nextList)
       if (attached) {
+        suppressAutoAttachRef.current = false
         void refreshDevices(bootedTarget)
       }

Option 2: Suppress retries on failure

       }
     } catch (e: unknown) {
+      suppressAutoAttachRef.current = true
       if (requestedTarget && liveTargetRef.current === requestedTarget) {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/renderer/src/components/emulator-pane/use-emulator-pane-session.ts` at
line 115, The suppressAutoAttachRef.current flag is being unconditionally reset
which causes infinite retry when attach() fails; update the attach flow in
use-emulator-pane-session so suppression is only cleared after a successful
attach (move the suppressAutoAttachRef.current = false to execute after attach()
succeeds) or alternatively set suppressAutoAttachRef.current = true inside the
attach() catch block before rethrowing/returning, and ensure the loading finally
block does not unconditionally trigger the auto-attach effect; adjust references
to suppressAutoAttachRef.current, attach(), and the attach() catch/finally
sections accordingly.

src/main/emulator/serve-sim-execution.ts (1)

32-47: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

TOCTOU race and world-writable path in ensureMacOpenShim.

This is a previously flagged security issue that remains unresolved. ensureMacOpenShim writes a shell script to a world-predictable /tmp/orca-serve-sim-open-shim/open path and prepends that directory to PATH for serve-sim. On multi-user macOS, another user could pre-create this path with malicious content. The write is also non-atomic (writeFileSync directly to the final path, not write-then-rename). Since the shim directory shadows /usr/bin/open, serve-sim processes inherit a potentially attacker-controlled open binary.

🔒 Recommended fix

Use a non-shared directory (e.g., app.getPath('sessionData') or app.getPath('userData')) instead of the static /tmp path, and write atomically:

-const MAC_OPEN_SHIM_DIR = join(tmpdir(), 'orca-serve-sim-open-shim')
+const MAC_OPEN_SHIM_DIR = join(app.getPath('userData'), 'serve-sim-open-shim')
 const MAC_OPEN_SHIM_PATH = join(MAC_OPEN_SHIM_DIR, 'open')
+const MAC_OPEN_SHIM_TEMP = join(MAC_OPEN_SHIM_DIR, 'open.tmp')

 function ensureMacOpenShim(): string | null {
   if (platform() !== 'darwin') {
     return null
   }
   try {
     mkdirSync(MAC_OPEN_SHIM_DIR, { recursive: true })
     const current = existsSync(MAC_OPEN_SHIM_PATH) ? readFileSync(MAC_OPEN_SHIM_PATH, 'utf8') : ''
     if (current !== MAC_OPEN_SHIM) {
-      writeFileSync(MAC_OPEN_SHIM_PATH, MAC_OPEN_SHIM, { mode: 0o755 })
+      writeFileSync(MAC_OPEN_SHIM_TEMP, MAC_OPEN_SHIM, { mode: 0o755 })
+      renameSync(MAC_OPEN_SHIM_TEMP, MAC_OPEN_SHIM_PATH)
     }
     chmodSync(MAC_OPEN_SHIM_PATH, 0o755)
     return MAC_OPEN_SHIM_DIR

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/emulator/serve-sim-execution.ts` around lines 32 - 47, The
ensureMacOpenShim function currently writes MAC_OPEN_SHIM to a predictable,
shared MAC_OPEN_SHIM_DIR/MAC_OPEN_SHIM_PATH with non-atomic write and
world-writable risk; change it to create the shim inside a per-user, non-shared
directory (use Electron app.getPath('sessionData') or app.getPath('userData')
instead of the static MAC_OPEN_SHIM_DIR), ensure the directory has restrictive
permissions (owner-only), write the file atomically (write to a temp file in
that same directory then rename to MAC_OPEN_SHIM_PATH), set explicit owner/mode
(chmodSync to 0o755 after rename), and keep the function name ensureMacOpenShim
and constants MAC_OPEN_SHIM_PATH/MAC_OPEN_SHIM for locating the change; also
preserve the darwin guard and catch errors as before.

src/main/emulator/serve-sim-endpoint-readiness.ts (1)

41-41: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

socket.setTimeout is idle timeout, not connect timeout.

This is a previously flagged issue that remains unresolved. socket.setTimeout(CONNECT_TIMEOUT_MS, ...) in Node.js fires on idle timeout (time since last I/O), not on connection timeout. During connect(), the socket is actively establishing the TCP handshake and is not "idle" — so this timer may not fire at all during connection attempts. If serve-sim's port is not open, the OS-level TCP connect timeout (often 30–75s on macOS) applies instead of the intended 500ms, delaying the readiness retry loop in waitForServeSimEndpointReady.

🐛 Proposed fix

Use a separate setTimeout for the connection-establishment deadline:

 function canConnectToEndpoint(endpoint: TcpEndpoint): Promise<boolean> {
   return new Promise((resolve) => {
     const socket = connect({ host: endpoint.host, port: endpoint.port })
     socket.unref()
     let settled = false
     const finish = (ready: boolean) => {
       if (settled) {
         return
       }
       settled = true
+      clearTimeout(deadline)
       socket.removeAllListeners()
       socket.destroy()
       resolve(ready)
     }
-    socket.setTimeout(CONNECT_TIMEOUT_MS, () => finish(false))
+    const deadline = setTimeout(() => finish(false), CONNECT_TIMEOUT_MS)
     socket.once('connect', () => finish(true))
     socket.once('error', () => finish(false))
   })
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/emulator/serve-sim-endpoint-readiness.ts` at line 41, The current
use of socket.setTimeout(CONNECT_TIMEOUT_MS, ...) is wrong because it’s an idle
timer; replace it with a separate connection-establishment deadline: create a
plain setTimeout (e.g. connectTimer) that fires after CONNECT_TIMEOUT_MS to
destroy the socket and call finish(false), and clear that timer on 'connect',
'error', and when finish runs to avoid leaks; update the code inside
waitForServeSimEndpointReady/serve-sim-endpoint-readiness where socket and
finish() are referenced to remove the socket.setTimeout call and wire the new
connectTimer lifecycle to the existing socket event handlers and finish()
cleanup.

src/main/emulator/emulator-bridge.ts (2)

134-134: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

State corruption: activeByWorktree.delete before managed-only guard.

This is a previously flagged issue that remains unresolved. When managedOnly is true and the session is unmanaged (terminal-started), line 135 returns null — but line 134 has already deleted the worktree→session binding, which is unrecoverable. Subsequent CLI commands fail with emulator_no_active even though the serve-sim process still runs and the sessions Map still holds the entry.

🐛 Proposed fix

Move the delete to after the guard:

 private async stopActiveForWorktreeInternal(
   worktreeId: string,
   options: { shutdownDevice?: boolean; managedOnly?: boolean } = {}
 ): Promise<string | null> {
   const key = this.activeByWorktree.get(worktreeId)
   if (!key) {
     return null
   }
   const session = this.sessions.get(key)
-  this.activeByWorktree.delete(worktreeId)
   if (!session || (options.managedOnly && !session.managed)) {
     return null
   }
+  this.activeByWorktree.delete(worktreeId)
   await this.stopServeSimForDevice(session.deviceUdid, {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/emulator/emulator-bridge.ts` at line 134, The deletion of the
worktree→session mapping (this.activeByWorktree.delete(worktreeId)) is happening
before the managed-only guard and can corrupt state when managedOnly is true;
move the delete so it occurs after the check that returns null for unmanaged
sessions (the guard that reads managedOnly and session.managed) so the mapping
is only removed when you actually decide the session should be discarded; update
the logic in the method that uses activeByWorktree, sessions, managedOnly and
session.managed to perform the guard first and only call
this.activeByWorktree.delete(worktreeId) after confirming the session is allowed
to be removed.

---

334-348: ⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoff

Unconditional device shutdown on app quit may surprise users.

This is a previously flagged issue that remains unaddressed. destroyAllSessions() unconditionally calls shutdownSimulatorDevice() for every managed session on app quit, which powers off the iOS Simulator itself — not just the streaming helper. A user with a booted simulator (started outside Orca or for other purposes) may find it shut down after closing the app.

Consider separating "kill streaming helper" from "shut down simulator device" in the quit path, or making device shutdown opt-in via user preference.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/emulator/emulator-bridge.ts` around lines 334 - 348, The current
destroyAllSessions() unconditionally calls shutdownSimulatorDevice() for each
managed session; change it to only stop the streaming helper and make simulator
shutdown opt-in: remove the unconditional shutdownSimulatorDevice(...) call from
destroyAllSessions() so it only calls stopServeSimForDevice(session.deviceUdid,
{ helperPid: session.pid }) and handles errors, then either (a) add a new
boolean parameter to destroyAllSessions(shutdownDevices = false) that triggers
shutdownSimulatorDevice(...) when true, or (b) add a separate method (e.g.,
shutdownAllDevicesForManagedSessions()) that iterates sessions and calls
shutdownSimulatorDevice(...) so callers can invoke it based on a user preference
or startup-tracking flag (e.g., session.startedByOrca). Ensure sessions.clear()
and activeByWorktree.clear() behavior remains unchanged and that
stopServeSimForDevice and shutdownSimulatorDevice error handling is preserved.

src/renderer/src/components/emulator-pane/use-emulator-pane-controls.ts (1)

29-37: 💤 Low value

Rotation only cycles between two orientations.

sendRotate toggles between 'landscape_left' and 'portrait', omitting 'landscape_right' and 'portrait_upside_down'. If full rotation support is needed, consider cycling through all four orientations.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/renderer/src/components/emulator-pane/use-emulator-pane-controls.ts`
around lines 29 - 37, sendRotate currently toggles only between 'landscape_left'
and 'portrait', skipping 'landscape_right' and 'portrait_upside_down'; update
sendRotate (and the nextRotateOrientationRef initialization if present) to cycle
through a full ordered list of orientations (e.g.,
['portrait','landscape_left','portrait_upside_down','landscape_right']) instead
of a binary toggle, pick the next index modulo length, and pass that orientation
into callRuntimeRpc so all four orientations are supported.

src/cli/handlers/emulator.ts (1)

34-75: 💤 Low value

Clarify the dual input format for --points.

Lines 41-44 accept both { "points": [...] } and [...] as valid input. For a CLI flag, accepting two different JSON formats can be confusing. Consider supporting only the direct array format [...], or add a comment explaining why both are needed.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cli/handlers/emulator.ts` around lines 34 - 75, The parser
parseEmulatorGesturePoints currently accepts two input shapes for --points
(either a raw array or an object with a points property), which is confusing;
pick one consistent format and enforce it (preferred: require the direct array
form) by changing the parsing logic to expect an array at top-level and throw a
RuntimeClientError if the parsed value is not an array, remove the branch that
looks for parsed.points, and update the error message to reference --points
array format; update any related doc/comment near parseEmulatorGesturePoints to
explain the single accepted JSON shape.

src/main/runtime/rpc/methods/emulator.ts (1)

5-5: ⚡ Quick win

Remove redundant .optional() when using .partial().

In Zod 4, calling .partial() on an object already makes all keys optional. The additional .optional() on the field is redundant: z.object({ worktree: z.string().optional() }).partial() produces { worktree?: string | undefined }, where the .optional() adds no value.

♻️ Simplify the schema definitions

-const WorktreeParam = z.object({ worktree: z.string().optional() }).partial()
+const WorktreeParam = z.object({ worktree: z.string() }).partial()

And at line 134:

-    params: z.object({ worktree: z.string().optional() }).partial(),
+    params: z.object({ worktree: z.string() }).partial(),

Also applies to: 134-134

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/runtime/rpc/methods/emulator.ts` at line 5, The WorktreeParam schema
uses z.object({ worktree: z.string().optional() }).partial(), which redundantly
marks the field optional twice; remove the inner .optional() so it becomes
z.object({ worktree: z.string() }).partial() and do the same for any other
schema in this file that wraps a z.string().optional() inside a .partial() (the
similar definition referenced in the review), i.e., locate the WorktreeParam
constant and the other schema and drop the inner .optional() calls.

src/main/emulator/emulator-bridge.ts (2)

162-162: 💤 Low value

Remove pointless void statement.

Line 162 uses void _opts?.worktreeId to suppress an unused-parameter warning, but worktreeId is actively used at lines 169-173. The void statement has no effect.

♻️ Proposed fix

-  void _opts?.worktreeId
   if (_opts?.device) {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/emulator/emulator-bridge.ts` at line 162, Remove the pointless `void
_opts?.worktreeId` statement: delete that line and leave the `_opts` parameter
and later uses of `_opts.worktreeId` (lines where `worktreeId` is referenced)
intact so the actual value is used; if you intended to silence an
unused-parameter warning instead, rename the parameter or adjust linter settings
rather than using the `void` expression.

---

1-354: ⚖️ Poor tradeoff

Consider splitting this file to address max-lines.

Static analysis reports the file exceeds the line limit. As per coding guidelines, avoid adding a max-lines disable comment. Instead, consider extracting focused modules (e.g., session management, command routing, lifecycle/cleanup helpers) into separate files.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/emulator/emulator-bridge.ts` around lines 1 - 354, File exceeds
max-lines; split responsibilities out of EmulatorBridge by extracting session
management (move sessions map and methods registerActiveEmulator,
unregisterActiveEmulator, getActiveForWorktree, getReusableActiveForWorktree,
stopActiveForWorktreeInternal, stopActiveForWorktree,
stopActiveManagedForWorktree, shutdownActiveManagedForWorktree,
destroyAllSessions, onAppQuit, and uses of activeByWorktree) into a
SessionManager module/class, extract command routing/target resolution
(getTargetOrThrow, ensureUdid, ensureDeviceBooted, tap, gesture, type, button,
rotate, exec, execServeSim and parse/strip helpers) into a CommandRouter module
that the bridge delegates to, and move lifecycle/cleanup helpers
(stopServeSimForDevice, hasServeSimHelperForDevice, startHelperForDevice, kill,
shutdown and helper-process calls like
killServeSimHelperProcessesForDevice/listServeSimHelperProcessesForDevice) into
a LifecycleManager module; keep EmulatorBridge as a thin coordinator that
composes these modules in its constructor (inject ServeSimExecutable and
waitForEndpointReady) and preserves the existing public method signatures so
callers are unchanged.

Source: Coding guidelines

src/renderer/src/components/tab-bar/group-tab-order.ts (1)

85-90: 💤 Low value

Consider adding a comment explaining the simulator id pattern.

The simulator branch uses tab.id for both the presence check and the result id, differing from terminal/browser (which use tab.entityId for id). While this is correct because simulators have a 1:1 relationship between tabs and entities (so entityId === id), a brief comment would clarify why this branch follows a different pattern.

📝 Suggested clarifying comment

     seenBrowsers.add(tab.entityId)
     result.push({ type: 'browser', id: tab.entityId, tabId: tab.id })
+  // Simulator tabs have a 1:1 relationship with their entity, so we key by tab id.
   } else if (tab.contentType === 'simulator') {
     if (!simulatorTabIds.has(tab.id) || seenSimulators.has(tab.id)) {
       continue

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/renderer/src/components/tab-bar/group-tab-order.ts` around lines 85 - 90,
Add a short inline comment in the simulator branch (around the checks for
simulatorTabIds, seenSimulators and the result.push call) explaining that
simulators use tab.id as the entity id (i.e., entityId === id) and therefore we
intentionally use tab.id here instead of tab.entityId; reference the symbols
simulatorTabIds, seenSimulators, and the result.push({ type: 'simulator', id:
tab.id, tabId: tab.id }) so future readers understand the 1:1 relationship and
why this branch differs from terminal/browser handling.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/renderer/src/components/Terminal.tsx (1)

213-213: ⚡ Quick win

Document the opt-out setting semantics.

!== false is carrying a non-obvious contract here: older persisted settings default to enabled. A short Why: comment would make this much harder to "simplify" into a truthy check later.

Suggested change

-  const mobileEmulatorEnabled = useAppStore((s) => s.settings?.mobileEmulatorEnabled !== false)
+  // Why: this setting is opt-out so older sessions without a stored value stay enabled.
+  const mobileEmulatorEnabled = useAppStore((s) => s.settings?.mobileEmulatorEnabled !== false)

As per coding guidelines, "When writing or modifying code driven by a design doc or non-obvious constraint, add a comment explaining why the code behaves the way it does."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/renderer/src/components/Terminal.tsx` at line 213, The expression
computing mobileEmulatorEnabled in Terminal.tsx uses useAppStore((s) =>
s.settings?.mobileEmulatorEnabled !== false) and relies on an opt-out semantics
(undefined/old persisted values should default to enabled), so add a short
inline comment above that line explaining this non-obvious contract and why !==
false is used instead of a simple truthy check — reference
mobileEmulatorEnabled, useAppStore and settings?.mobileEmulatorEnabled in the
comment so future maintainers don't simplify it incorrectly.

Source: Coding guidelines