Performance improvements + Swift Concurrency - #117
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR converts serve-sim’s native capture, encoding, and HID paths to actor-based async APIs, switches capture delivery to codec-specific subscriptions, and propagates async handling through middleware, runtime, device streaming, dev tooling, and build settings. ChangesServe-sim actor pipeline and async wiring
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kabiroberai
commented
Jun 24, 2026
| frontServer.once("listening", onListening); | ||
| frontServer.listen(opts.port, opts.host ?? "127.0.0.1"); | ||
| }); | ||
| // const frontServer = createPreviewFrontServer(opts.middleware, internalAddress.port); |
Contributor
Author
There was a problem hiding this comment.
feel like we should ideally merge frontServer into the internal server if possible (I'm not sure if this is vestigial, maybe the websocket being able to access the raw socket was load bearing to fix the issue with Bun websocket proxying? though if we switch to Node this shouldn't be necessary.)
kabiroberai
commented
Jun 24, 2026
| // this.lastTimeout = undefined; | ||
| // } | ||
| // this.lastTimeout = setTimeout(() => { | ||
| // if (global.gc) global.gc({ execution: "sync", flavor: "regular" }) |
Contributor
Author
There was a problem hiding this comment.
debounced gc on idle, couldn't figure out if this really helped
kabiroberai
commented
Jun 24, 2026
| self.queue = queue | ||
|
|
||
| let rawData = NSMutableData(length: 1024 * 1024 * 10)! | ||
| let buffer = try NodeArrayBuffer(data: rawData) |
Contributor
Author
There was a problem hiding this comment.
this is critical: it allows us to reuse the same buffer across calls. One thing it changes though is that the callee has to "use" the current contents of the buffer immediately, because they'll be overwritten later.
kabiroberai
commented
Jun 24, 2026
| private var screenHeight = 0 | ||
| private var encoderReady = false | ||
| private var encoding = false // MJPEG backpressure | ||
| private var h264Encoding = false // H.264 backpressure |
Contributor
Author
There was a problem hiding this comment.
we apply backpressure via async instead
kabiroberai
commented
Jun 24, 2026
| } | ||
|
|
||
| @objc protocol FramebufferDescriptor { | ||
| @objc(registerScreenCallbacksWithUUID:callbackQueue:frameCallback:surfacesChangedCallback:propertiesChangedCallback:) |
Contributor
Author
There was a problem hiding this comment.
cool trick that we should use elsewhere too: if you declare a method in an @objc protocol you're able to call it on any AnyObject. Also enables CF-NS-Swift toll-free bridging.
kabiroberai
commented
Jun 24, 2026
|
|
||
| OUT="$OUT_DIR/${PRODUCT}.node" | ||
| lipo -create -output "$OUT" "$arm64_dylib" "$x64_dylib" | ||
| cp -a "$DYLIB" "$OUT" |
Contributor
Author
There was a problem hiding this comment.
these changes are from #116, but don't think I can create a stacked PR where both branches are in my fork
EvanBacon
reviewed
Jun 24, 2026
| }); | ||
| const internalServer = createHttpServer( | ||
| { | ||
| highWaterMark: 1024 * 1024 * 10, |
Owner
There was a problem hiding this comment.
How did we land on 10mb of back pressure tolerance?
Contributor
Author
There was a problem hiding this comment.
completely unscientific tbh. feel free to change. Primarily wanted to make sure it was large enough to mean no dropped frames on localhost, but even then 10M is possibly overkill since frames are usually <500K.
kabiroberai
force-pushed
the
perf-improvements
branch
5 times, most recently
from
June 28, 2026 04:42
dd77b2c to
53de63f
Compare
kabiroberai
changed the title
kabiroberai
changed the title
kabiroberai
force-pushed
the
perf-improvements
branch
2 times, most recently
from
June 28, 2026 04:55
ab4214b to
43ad04b
Compare
kabiroberai
commented
Jun 28, 2026
| return bootedSnapshot.booted; | ||
| } | ||
| try { | ||
| const output = execSync("xcrun simctl list devices booted -j", { |
Contributor
Author
There was a problem hiding this comment.
this turned out to be a major source of hitches. the stream was freezing for ~500ms every ~5s because we would block the event loop with a synchronous call to simctl list
kabiroberai
marked this pull request as ready for review
June 28, 2026 04:58
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43ad04bb4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+182
to
+184
| while true { | ||
| guard let self else { return } | ||
| await self.onIdleTimerTick() |
There was a problem hiding this comment.
In any running capture session, this replacement for the DispatchSourceTimer loops as fast as each actor hop can complete because there is no Task.sleep and cancellation is never checked; idleTimer?.cancel() in stop() therefore will not stop the loop either. This will peg a CPU core and continuously enqueue onIdleTimerTick() work even when no idle frame is due, so the loop should sleep for the idle interval and exit on cancellation.
Useful? React with 👍 / 👎.
Comment on lines
+103
to
+104
| Task { | ||
| try await frameCapture.start(deviceUDID: deviceUDID) { pixelBuffer, _ in |
There was a problem hiding this comment.
When /helper is opened for a stale or non-booted UDID, frameCapture.start throws inside this unobserved task, but CaptureEngine.start() has already returned and then marks the engine running. That means getDeviceSession() no longer catches startup failure as intended, so the helper can report healthy while streams just hang with no frames; the startup task needs to be awaited or otherwise surface its error before entering the running phase.
Useful? React with 👍 / 👎.
EvanBacon
previously approved these changes
Jun 28, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/serve-sim/Sources/SimNative/HIDInjector.swift (1)
319-319: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTie the delayed scroll-end to the latest gesture state.
scrollEndWork?.cancel()does not stop aDispatchWorkItemthat is later executed withperform(), so an older sleep can still fire and end a newer drag early. Use a generation/token check before sending the finalend.🤖 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 `@packages/serve-sim/Sources/SimNative/HIDInjector.swift` at line 319, The delayed scroll-end logic in HIDInjector’s scroll handling can still complete an older gesture after a newer drag has started because canceling scrollEndWork does not block a later perform() from running. Update the scroll-end scheduling path around the scrollEndWork DispatchWorkItem to tie each pending end action to the current gesture state, using a generation/token check before sending the final end so only the latest drag can finish.packages/serve-sim/src/middleware.ts (2)
1782-1804: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep
/appstatelog tail setup from being skipped.Line 1785 now returns from the whole middleware when the bootstrap bundle is missing/non-user-facing, so the log stream is never spawned. Since the bootstrap is awaited before the
req.closehandler andspawn, slow AX/RN detection can also miss foreground events or start work after disconnect.Proposed fix
// `proc_pidpath`+Info.plist resolution and emit it before tailing. let lastBundle = ""; - try { - const info = JSON.parse(await axFrontmostAsync(udid)) as { bundleId?: string; pid?: number }; - if (!info.bundleId || !isUserFacingBundle(info.bundleId)) return; - if (res.writableEnded) return; - lastBundle = info.bundleId; - const isReactNative = await detectReactNative(udid, info.bundleId); - if (res.writableEnded) return; - res.write("data: " + JSON.stringify({ bundleId: info.bundleId, pid: info.pid, isReactNative }) + "\n\n"); - } catch { - // AX bridge may be warming up — the log tail fills in once anything moves. - } + let closed = false; + void (async () => { + try { + const info = JSON.parse(await axFrontmostAsync(udid)) as { bundleId?: string; pid?: number }; + if (!info.bundleId || !isUserFacingBundle(info.bundleId)) return; + if (closed || res.writableEnded) return; + lastBundle = info.bundleId; + const isReactNative = await detectReactNative(udid, info.bundleId); + if (closed || res.writableEnded) return; + res.write("data: " + JSON.stringify({ bundleId: info.bundleId, pid: info.pid, isReactNative }) + "\n\n"); + } catch { + // AX bridge may be warming up — the log tail fills in once anything moves. + } + })(); const child: ChildProcess = spawn("xcrun", [ @@ - let closed = false; const emitApp = async (bundleId: string, pid?: number) => {🤖 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 `@packages/serve-sim/src/middleware.ts` around lines 1782 - 1804, The /appstate middleware flow is returning too early from the initial bundle check, which skips the log tail setup entirely. In middleware.ts, adjust the control flow around the axFrontmostAsync / detectReactNative block so missing or non-user-facing bootstrap bundles only skip the immediate SSE emit, not the later simctl log stream spawn and req.close wiring. Keep the spawn("xcrun", ...) log tail initialization and related handlers reachable even when the bootstrap info is absent or delayed.
1393-1403: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInvalidate the booted-device cache after shutdown.
After a successful
simctl shutdown,readServeSimStates()can still see the oldbootedSnapshotfor up to 1.5s and skip recycling the stale helper.Proposed fix
} + bootedSnapshot = { at: 0, booted: null }; res.writeHead(200, { "Content-Type": "application/json" }); res.end(JSON.stringify({ ok: true }));🤖 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 `@packages/serve-sim/src/middleware.ts` around lines 1393 - 1403, Invalidate the booted-device cache after a successful shutdown in the simctl shutdown handler inside middleware.ts. Update the logic around the execFile("xcrun", ["simctl", "shutdown", udid], ...) callback so that when shutdown succeeds, the stale bootedSnapshot used by readServeSimStates() is cleared or marked invalid before returning 200, ensuring the helper is not skipped due to cached booted state. Use the shutdown flow in the middleware handler and the readServeSimStates()/bootedSnapshot path as the key places to update.packages/serve-sim/dev.ts (1)
254-265: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAwait the HTML render and return the delegated middleware call.
buildHtml()returns a Promise, sores.end(buildHtml(device))sends the promise instead of the HTML string. Returningmiddleware(req, res, next)keeps this async wrapper aligned with the middleware contract.🤖 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 `@packages/serve-sim/dev.ts` around lines 254 - 265, The devMiddleware wrapper is sending a Promise to res.end and not returning the delegated middleware call. Update devMiddleware to await buildHtml(device) before ending the response, and return the middleware(req, res, next) call so the async middleware contract is preserved; use devMiddleware and buildHtml as the key symbols to locate the fix.
🧹 Nitpick comments (1)
packages/serve-sim/Sources/SimNative/HIDInjector.swift (1)
152-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale
inputQueuecontract comments.The queue was removed in favor of actor isolation, but these comments still instruct future callers to reason about
inputQueue.♻️ Proposed wording cleanup
- /// Must run on `inputQueue`. + /// Must run on the `HIDInjector` actor executor. - /// to call off `inputQueue`. NSSize(1,1) makes ratio = point. + /// to call while building HID messages. NSSize(1,1) makes ratio = point. - /// already running on `inputQueue`. + /// already running on the `HIDInjector` actor executor. - /// `inputQueue` block (button sequences below run there). + /// actor-isolated button sequence. - /// touch-down before the finger moves. Runs on `inputQueue`. + /// touch-down before the finger moves.Also applies to: 164-166, 178-180, 233-235, 325-327
🤖 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 `@packages/serve-sim/Sources/SimNative/HIDInjector.swift` around lines 152 - 154, Update the stale concurrency contract comments in HIDInjector.swift: the `rawSend(_:)` and related methods still mention `inputQueue`, but the queue has been removed in favor of actor isolation. Replace those comments with wording that reflects actor-based access and remove any guidance that tells callers to reason about `inputQueue`, including the other affected doc comments referenced in the review.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/serve-sim/Sources/SimNative/CaptureEngine.swift`:
- Around line 87-108: The CaptureEngine.start flow is marking the engine as
running even when frameCapture.start fails because it is launched in an
unobserved Task. Update start() so the frameCapture.start(deviceUDID:callback:)
error is awaited and propagated before setting phase to .running, and keep the
phase at .starting/.unstarted on failure so retries remain possible; use the
start() method, frameCapture.start, and phase transitions in CaptureEngine as
the key points to adjust.
- Around line 175-183: The CaptureEngine.stop() implementation is not actually
draining before returning because it spawns frameCapture.stop() in a detached
Task and immediately clears consumers, which can leave in-flight encode/onFrame
callbacks running. Update stop() so it synchronously waits for
frameCapture.stop() to complete before returning, and make sure any active
consumer/callback state is quiesced in CaptureEngine.stop() itself so no
callbacks can fire after the method exits.
- Around line 34-50: The frame callback in CaptureEngine is using shared engine
state for dimensions, which can drift while older frames are still being
encoded. Update the capture/encoding flow so the dimensions come from the same
encoded frame being delivered (for example via the Encoded value produced by
encoder.encode), and thread that through the onFrame callback instead of reading
screenSize later. Apply the same fix anywhere else in CaptureEngine where
encoded output is paired with dimensions so the bytes and size always match.
In `@packages/serve-sim/Sources/SimNative/FrameCapture.swift`:
- Around line 246-250: The frame accounting in FrameCapture’s capture path
increments frameCount before verifying that photocopier.copy(pb) succeeded,
which can trip the “never captured” recovery path even when no frame is emitted.
Move the frameCount increment and timestamp creation in the capture flow so they
happen only after the deep copy succeeds, keeping lastCaptureTime/onFrame tied
to a successful copy in FrameCapture.
- Around line 180-192: The idle loop in startIdleTimer() is spinning
continuously and does not observe Task cancellation, so stop() cannot terminate
it cleanly. Update the loop to periodically suspend and check for cancellation
inside startIdleTimer (and any helper like onIdleTimerTick if needed), using the
existing idleTimer Task and self.captureFrame(force:) flow so the task exits
when cancelled instead of pegging CPU.
In `@packages/serve-sim/Sources/SimNative/H264Encoder.swift`:
- Around line 63-82: `H264Encoder.encodeFrame` can hang forever because
`VTCompressionSessionEncodeFrame` may return success without ever calling the
output callback, leaving the `withCheckedContinuation` unresolved. Add a
timeout/cancellation path in this encode flow, using the existing timeout
constant from `AVCCEncoder` so stalled encodes resume with an error instead of
blocking AVCC delivery. Make sure the fix is localized around the
`VTCompressionSessionEncodeFrame` call and its continuation handling in
`H264Encoder`.
In `@packages/serve-sim/Sources/SimNative/PixelBufferUtils.swift`:
- Around line 45-50: The PixelBuffer lock/unlock flow in PixelBufferUtils should
verify the return status from CVPixelBufferLockBaseAddress before proceeding.
Update the logic around the source and dst locking so that if either lock call
fails, the function exits early without copying or reaching the defer-based
unlocks, and only unlock buffers that were successfully locked.
In `@packages/serve-sim/Sources/SimNative/sim-module.swift`:
- Around line 27-31: The SimNative constructor currently starts HID setup in a
fire-and-forget Task, so setup errors are lost and NodeMethod calls can run
before the injector is ready. Update SimNative.init and the related NodeMethod
entry points to ensure HIDInjector.setup(deviceUDID:) completes before any
method is exposed, either by awaiting setup during construction or by storing
the setup task and awaiting it in each method before touching injector.
In `@packages/serve-sim/Sources/SimNative/VideoEncoder.swift`:
- Around line 20-28: In VideoEncoder.encode(pixelBuffer:), the pixel buffer is
locked without checking whether CVPixelBufferLockBaseAddress succeeded. Update
the lock step to validate the return value before using
CVPixelBufferGetBaseAddress or registering the unlock defer, and throw an
appropriate error from Errors if locking fails. Use the encode(pixelBuffer:)
method and Errors.invalidPixelBuffer as the main anchors when applying the fix.
In `@packages/serve-sim/src/device-session.ts`:
- Around line 178-184: The subscribed MJPEG path in device-session’s streaming
logic still always uses writeMjpegFrame, so raw=1 responses incorrectly get
multipart boundaries. Update the async frame పంపing flow around latestJpeg,
subscribeMjpeg, and writeMjpegFrame to branch on the raw mode: when raw=1, write
the frame bytes directly to res for both the immediate frame and each subscribed
frame; otherwise keep using writeMjpegFrame for multipart MJPEG output.
- Around line 181-187: `DeviceSession.subscribeMjpeg`/`subscribeAvcc` can still
write after the response has been closed because `waitForDrain()` may resume on
`close`/`error`; add a `res.destroyed`/`res.writableEnded` guard immediately
after `waitForDrain()` and before `writeMjpegFrame`/`writeAvccFrame`, and make
the `close`/`error` handlers unsubscribe safely even if the subscription
resolves after the response is already gone. Keep the fix localized around the
`subscribeMjpeg`, `subscribeAvcc`, and `waitForDrain` flow in
`device-session.ts`.
In `@packages/serve-sim/src/native.ts`:
- Line 38: The native wrapper types are still synchronous even though
sim-module.swift exposes async start/stop and an async unsubscribe from
subscribe, so update the relevant declarations in native.ts to model those
lifecycle paths as async. Adjust the subscribe signature and any related
start/stop interfaces or implementations to return async Promise-based handles,
and make sure callers in the native bridge await the returned unsubscribe
function so startup/teardown failures are preserved and cleanup is not raced.
Use the existing subscribe and lifecycle symbols in the native wrapper to keep
the fix aligned with the bridge API.
---
Outside diff comments:
In `@packages/serve-sim/dev.ts`:
- Around line 254-265: The devMiddleware wrapper is sending a Promise to res.end
and not returning the delegated middleware call. Update devMiddleware to await
buildHtml(device) before ending the response, and return the middleware(req,
res, next) call so the async middleware contract is preserved; use devMiddleware
and buildHtml as the key symbols to locate the fix.
In `@packages/serve-sim/Sources/SimNative/HIDInjector.swift`:
- Line 319: The delayed scroll-end logic in HIDInjector’s scroll handling can
still complete an older gesture after a newer drag has started because canceling
scrollEndWork does not block a later perform() from running. Update the
scroll-end scheduling path around the scrollEndWork DispatchWorkItem to tie each
pending end action to the current gesture state, using a generation/token check
before sending the final end so only the latest drag can finish.
In `@packages/serve-sim/src/middleware.ts`:
- Around line 1782-1804: The /appstate middleware flow is returning too early
from the initial bundle check, which skips the log tail setup entirely. In
middleware.ts, adjust the control flow around the axFrontmostAsync /
detectReactNative block so missing or non-user-facing bootstrap bundles only
skip the immediate SSE emit, not the later simctl log stream spawn and req.close
wiring. Keep the spawn("xcrun", ...) log tail initialization and related
handlers reachable even when the bootstrap info is absent or delayed.
- Around line 1393-1403: Invalidate the booted-device cache after a successful
shutdown in the simctl shutdown handler inside middleware.ts. Update the logic
around the execFile("xcrun", ["simctl", "shutdown", udid], ...) callback so that
when shutdown succeeds, the stale bootedSnapshot used by readServeSimStates() is
cleared or marked invalid before returning 200, ensuring the helper is not
skipped due to cached booted state. Use the shutdown flow in the middleware
handler and the readServeSimStates()/bootedSnapshot path as the key places to
update.
---
Nitpick comments:
In `@packages/serve-sim/Sources/SimNative/HIDInjector.swift`:
- Around line 152-154: Update the stale concurrency contract comments in
HIDInjector.swift: the `rawSend(_:)` and related methods still mention
`inputQueue`, but the queue has been removed in favor of actor isolation.
Replace those comments with wording that reflects actor-based access and remove
any guidance that tells callers to reason about `inputQueue`, including the
other affected doc comments referenced in the review.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b522e86a-04a2-405f-a211-826e41d170c6
📒 Files selected for processing (15)
packages/serve-sim/Sources/SimNative/CaptureEngine.swiftpackages/serve-sim/Sources/SimNative/FrameCapture.swiftpackages/serve-sim/Sources/SimNative/H264Encoder.swiftpackages/serve-sim/Sources/SimNative/HIDInjector.swiftpackages/serve-sim/Sources/SimNative/PixelBufferUtils.swiftpackages/serve-sim/Sources/SimNative/VideoEncoder.swiftpackages/serve-sim/Sources/SimNative/sim-capture.swiftpackages/serve-sim/Sources/SimNative/sim-module.swiftpackages/serve-sim/build.tspackages/serve-sim/dev.tspackages/serve-sim/src/__tests__/exec-auth.test.tspackages/serve-sim/src/device-session.tspackages/serve-sim/src/middleware.tspackages/serve-sim/src/native.tspackages/serve-sim/src/runtime.ts
💤 Files with no reviewable changes (1)
- packages/serve-sim/Sources/SimNative/sim-capture.swift
Comment on lines
+34
to
+50
| init( | ||
| encoder: E, | ||
| onFrame: @escaping @isolated(any) (E.Encoded) async -> Void | ||
| ) { | ||
| let (stream, continuation) = AsyncStream.makeStream( | ||
| of: Frame.self, | ||
| // drop old frames if there's backpressure | ||
| bufferingPolicy: .bufferingNewest(1) | ||
| ) | ||
| self.continuation = continuation | ||
| Task { | ||
| _ = onFrame.isolation | ||
| for await frame in stream { | ||
| do { | ||
| let encoded = try await encoder.encode(frame) | ||
| await onFrame(encoded) | ||
| } catch { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Send dimensions from the encoded frame, not shared engine state.
screenSize can advance while an older frame is still encoding, so callbacks may report dimensions for a different frame than the bytes being delivered.
Refactor direction
- onFrame: `@escaping` `@isolated`(any) (E.Encoded) async -> Void
+ onFrame: `@escaping` `@isolated`(any) (Frame, E.Encoded) async -> Void
...
- await onFrame(encoded)
+ await onFrame(frame, encoded)
...
- return addConsumer(encoder: mjpegEncoder, onFrame: { [weak self] data in
- guard let self else { return }
- await onFrame(screenSize, data)
+ return addConsumer(encoder: mjpegEncoder, onFrame: { frame, data in
+ await onFrame(frame.pixelBuffer.dimensions, data)Also applies to: 137-169
🤖 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 `@packages/serve-sim/Sources/SimNative/CaptureEngine.swift` around lines 34 -
50, The frame callback in CaptureEngine is using shared engine state for
dimensions, which can drift while older frames are still being encoded. Update
the capture/encoding flow so the dimensions come from the same encoded frame
being delivered (for example via the Encoded value produced by encoder.encode),
and thread that through the onFrame callback instead of reading screenSize
later. Apply the same fix anywhere else in CaptureEngine where encoded output is
paired with dimensions so the bytes and size always match.
Comment on lines
+175
to
+183
| /// Halt frame production and drain the encode queues so no callback can fire | ||
| /// after this returns — the N-API layer relies on that before releasing the | ||
| /// threadsafe function. | ||
| func stop() { | ||
| if phase == .stopped { return } | ||
| phase = .stopped | ||
| Task { [frameCapture] in await frameCapture.stop() } | ||
| consumers.removeAll() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Make stop() actually drain before returning.
The comment promises no callback can fire after stop(), but the implementation launches frameCapture.stop() in a task and drops consumers without awaiting in-flight encode/onFrame work.
🤖 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 `@packages/serve-sim/Sources/SimNative/CaptureEngine.swift` around lines 175 -
183, The CaptureEngine.stop() implementation is not actually draining before
returning because it spawns frameCapture.stop() in a detached Task and
immediately clears consumers, which can leave in-flight encode/onFrame callbacks
running. Update stop() so it synchronously waits for frameCapture.stop() to
complete before returning, and make sure any active consumer/callback state is
quiesced in CaptureEngine.stop() itself so no callbacks can fire after the
method exits.
Contributor
Author
There was a problem hiding this comment.
the phase == .running check before handleFrame should mitigate this
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/serve-sim/Sources/SimNative/CaptureEngine.swift"
ast-grep outline "$file" --view expanded || true
printf '\n--- CaptureEngine.swift lines 1-240 ---\n'
sed -n '1,240p' "$file"
printf '\n--- FrameCapture.swift outline + relevant stop/start lines ---\n'
ast-grep outline packages/serve-sim/Sources/SimNative/FrameCapture.swift --view expanded || true
sed -n '1,240p' packages/serve-sim/Sources/SimNative/FrameCapture.swift | sed -n '1,220p'Length of output: 18730
@kabiroberai, I don't think that guard fully closes the hole.
The phase == .running check on Line 94 only stops new frames from being forwarded from the engine-level stream. But each CaptureConsumer has its own internal AsyncStream and worker task started on Line 41, so a frame that was already handed to consumer.handleFrame(frame) on Line 130 can still be:
- sitting in the consumer buffer,
- inside
encoder.encode(...), or - already in
await onFrame(...)
when stop() returns.
So even after Line 180 sets phase = .stopped, stop() still:
- does not await
frameCapture.stop(), and - does not wait for consumer worker tasks to quiesce before returning.
That means the current implementation still doesn't satisfy the doc comment's guarantee that “no callback can fire after this returns.”
Comment on lines
27
to
31
| @NodeConstructor init(_ udid: String) throws { | ||
| self.udid = udid | ||
| injector = HIDInjector() | ||
| try injector.setup(deviceUDID: udid) | ||
| Task { try await injector.setup(deviceUDID: udid) } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files 'packages/serve-sim/Sources/SimNative/*' 'packages/serve-sim/Sources/SimNative/**/*' | sed -n '1,200p'
printf '\n== outline: sim-module.swift ==\n'
ast-grep outline packages/serve-sim/Sources/SimNative/sim-module.swift --view expanded || true
printf '\n== read sim-module.swift ==\n'
sed -n '1,260p' packages/serve-sim/Sources/SimNative/sim-module.swift
printf '\n== search for injector.setup / hidClient / simDevice / NodeMethod in SimNative ==\n'
rg -n "injector\.setup|hidClient|simDevice|`@NodeMethod`|Task\s*\{" packages/serve-sim/Sources/SimNative -S
printf '\n== read related HID file(s) if present ==\n'
for f in $(git ls-files 'packages/serve-sim/Sources/SimNative/*' | rg 'HID|hid|SimHID|sim-module'); do
echo "--- $f"
wc -l "$f"
doneRepository: EvanBacon/serve-sim
Length of output: 13654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== outline: HIDInjector.swift ==\n'
ast-grep outline packages/serve-sim/Sources/SimNative/HIDInjector.swift --view expanded || true
printf '\n== setup / send method ranges ==\n'
rg -n "func setup|func sendTouch|func sendMultiTouch|func sendButton\\(|func sendButtonHID|func sendKey|func sendScroll|func sendDigitalCrown|func sendOrientation|func simulateMemoryWarning|func toggleSoftwareKeyboard|func setCADebugOption|guard let client|guard let device|guard let sendSel" packages/serve-sim/Sources/SimNative/HIDInjector.swift -n -S
printf '\n== relevant excerpts ==\n'
sed -n '1,220p' packages/serve-sim/Sources/SimNative/HIDInjector.swift
printf '\n--- middle ---\n'
sed -n '220,380p' packages/serve-sim/Sources/SimNative/HIDInjector.swift
printf '\n--- later ---\n'
sed -n '380,620p' packages/serve-sim/Sources/SimNative/HIDInjector.swiftRepository: EvanBacon/serve-sim
Length of output: 33176
Await SimHID setup before exposing methods packages/serve-sim/Sources/SimNative/sim-module.swift:27-31
The detached Task drops setup errors, and calls made before it finishes just hit HIDInjector’s nil guards and no-op or return false. Keep the setup task and await it from each NodeMethod, or finish setup in the constructor so the object is usable as soon as it returns.
🤖 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 `@packages/serve-sim/Sources/SimNative/sim-module.swift` around lines 27 - 31,
The SimNative constructor currently starts HID setup in a fire-and-forget Task,
so setup errors are lost and NodeMethod calls can run before the injector is
ready. Update SimNative.init and the related NodeMethod entry points to ensure
HIDInjector.setup(deviceUDID:) completes before any method is exposed, either by
awaiting setup during construction or by storing the setup task and awaiting it
in each method before touching injector.
kabiroberai
force-pushed
the
perf-improvements
branch
2 times, most recently
from
June 28, 2026 05:17
b634189 to
f0a1499
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/serve-sim/Sources/SimNative/H264Encoder.swift (1)
44-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale
encodeAPI comment.Line 44 still describes the removed
onEncodedcallback contract, butencodenow suspends and returnsEncoded.Proposed wording
- /// Submit a frame. Returns immediately; `onEncoded` fires on VT's queue. + /// Submit a frame and await VideoToolbox's encoded sample. func encode(_ source: CVPixelBuffer, forceKeyframe: Bool = false) async throws -> Encoded {🤖 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 `@packages/serve-sim/Sources/SimNative/H264Encoder.swift` around lines 44 - 45, The `encode` API comment in `H264Encoder` is stale and still refers to the removed `onEncoded` callback contract. Update the documentation on `encode(_ source:forceKeyframe:)` to describe the current async behavior: it suspends until encoding completes and returns an `Encoded` value, with no callback mention.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/serve-sim/src/runtime.ts`:
- Around line 155-160: The async request handler inside the HTTP listener in
runtime.ts does not handle rejected middleware promises, so a failure from
opts.middleware can become an unhandled rejection and leave the response open.
Update the server callback around opts.middleware(req, res, ...) to explicitly
catch errors with .catch(...) or try/catch, and in the error path either set a
500 response or destroy the socket before exiting. Keep the existing 404
fallback behavior in the success path.
---
Nitpick comments:
In `@packages/serve-sim/Sources/SimNative/H264Encoder.swift`:
- Around line 44-45: The `encode` API comment in `H264Encoder` is stale and
still refers to the removed `onEncoded` callback contract. Update the
documentation on `encode(_ source:forceKeyframe:)` to describe the current async
behavior: it suspends until encoding completes and returns an `Encoded` value,
with no callback mention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c3a0634d-5f5c-4cfd-a2f8-83d2cd3ed405
📒 Files selected for processing (15)
packages/serve-sim/Sources/SimNative/CaptureEngine.swiftpackages/serve-sim/Sources/SimNative/FrameCapture.swiftpackages/serve-sim/Sources/SimNative/H264Encoder.swiftpackages/serve-sim/Sources/SimNative/HIDInjector.swiftpackages/serve-sim/Sources/SimNative/PixelBufferUtils.swiftpackages/serve-sim/Sources/SimNative/VideoEncoder.swiftpackages/serve-sim/Sources/SimNative/sim-capture.swiftpackages/serve-sim/Sources/SimNative/sim-module.swiftpackages/serve-sim/build.tspackages/serve-sim/dev.tspackages/serve-sim/src/__tests__/exec-auth.test.tspackages/serve-sim/src/device-session.tspackages/serve-sim/src/middleware.tspackages/serve-sim/src/native.tspackages/serve-sim/src/runtime.ts
💤 Files with no reviewable changes (1)
- packages/serve-sim/Sources/SimNative/sim-capture.swift
✅ Files skipped from review due to trivial changes (1)
- packages/serve-sim/build.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/serve-sim/src/tests/exec-auth.test.ts
- packages/serve-sim/Sources/SimNative/PixelBufferUtils.swift
- packages/serve-sim/Sources/SimNative/VideoEncoder.swift
- packages/serve-sim/dev.ts
- packages/serve-sim/Sources/SimNative/CaptureEngine.swift
- packages/serve-sim/Sources/SimNative/sim-module.swift
- packages/serve-sim/src/middleware.ts
- packages/serve-sim/Sources/SimNative/FrameCapture.swift
- packages/serve-sim/Sources/SimNative/HIDInjector.swift
kabiroberai
force-pushed
the
perf-improvements
branch
3 times, most recently
from
June 28, 2026 05:36
eef9d10 to
e4e92ad
Compare
kabiroberai
force-pushed
the
perf-improvements
branch
from
July 1, 2026 03:02
e4e92ad to
30d2220
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/serve-sim/src/middleware.ts`:
- Around line 1637-1643: The SSE handler in middleware.ts sets up the closed
cleanup too late, after the initial await on computeConfig/readServeSimStates,
so a disconnect during that wait can leak the watcher/heartbeat. Move the closed
flag and close listener registration before the first async recompute in the SSE
setup block, and make sure the existing sendIfChanged/res connection cleanup
logic uses that early guard so the stream can always be torn down correctly.
- Around line 1827-1834: The appstate SSE route in middleware.ts is returning
from the whole handler when axFrontmostAsync() yields no user-facing bundle,
which leaves the stream open but never starts the log tail. Update the appstate
handling in the middleware route so the bootstrap/SpringBoard case skips the
frontmost-app event but still continues into the log-tail startup logic instead
of exiting early. Use the existing symbols axFrontmostAsync, isUserFacingBundle,
lastBundle, and detectReactNative to keep the user-facing path unchanged while
allowing non-user-facing bootstrap apps to proceed.
- Around line 1254-1266: The DevTools asset proxy in middleware.ts is validating
the raw assetPath, but encoded traversal like %2e%2e can still escape when the
upstream URL is built. Update the assetPath handling in the middleware flow to
decode and validate path segments before the fetch call, and reject any decoded
“..” segments in the same place where the current split("/") check happens. Use
the existing assetPath, devtoolsFrontendBase, and fetch logic to keep the fix
localized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cfd7a744-2616-435e-90af-371b37213ef5
📒 Files selected for processing (15)
packages/serve-sim/Sources/SimNative/CaptureEngine.swiftpackages/serve-sim/Sources/SimNative/FrameCapture.swiftpackages/serve-sim/Sources/SimNative/H264Encoder.swiftpackages/serve-sim/Sources/SimNative/HIDInjector.swiftpackages/serve-sim/Sources/SimNative/PixelBufferUtils.swiftpackages/serve-sim/Sources/SimNative/VideoEncoder.swiftpackages/serve-sim/Sources/SimNative/sim-capture.swiftpackages/serve-sim/Sources/SimNative/sim-module.swiftpackages/serve-sim/build.tspackages/serve-sim/dev.tspackages/serve-sim/src/__tests__/exec-auth.test.tspackages/serve-sim/src/device-session.tspackages/serve-sim/src/middleware.tspackages/serve-sim/src/native.tspackages/serve-sim/src/runtime.ts
💤 Files with no reviewable changes (1)
- packages/serve-sim/Sources/SimNative/sim-capture.swift
✅ Files skipped from review due to trivial changes (2)
- packages/serve-sim/src/tests/exec-auth.test.ts
- packages/serve-sim/build.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/serve-sim/Sources/SimNative/PixelBufferUtils.swift
- packages/serve-sim/dev.ts
- packages/serve-sim/Sources/SimNative/CaptureEngine.swift
- packages/serve-sim/Sources/SimNative/VideoEncoder.swift
- packages/serve-sim/src/device-session.ts
- packages/serve-sim/src/native.ts
- packages/serve-sim/Sources/SimNative/sim-module.swift
- packages/serve-sim/src/runtime.ts
- packages/serve-sim/Sources/SimNative/H264Encoder.swift
- packages/serve-sim/Sources/SimNative/FrameCapture.swift
- packages/serve-sim/Sources/SimNative/HIDInjector.swift
Comment on lines
+1254
to
+1266
| const assetPath = url === devtoolsFrontendBase | ||
| ? "inspector.html" | ||
| : url.slice(devtoolsFrontendBase.length + 1); | ||
| // Reject path-traversal segments before they reach the upstream URL. | ||
| if (assetPath.split("/").some((seg) => seg === "..")) { | ||
| res.writeHead(400, { "Content-Type": "text/plain; charset=utf-8" }); | ||
| res.end("Invalid asset path"); | ||
| return; | ||
| } | ||
| try { | ||
| const upstream = await fetch( | ||
| `https://chrome-devtools-frontend.appspot.com/serve_rev/@${DEVTOOLS_FRONTEND_REV}/${assetPath}${qIndex === -1 ? "" : rawUrl.slice(qIndex)}`, | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify encoded dot segments are normalized by Node's URL parser.
node - <<'NODE'
const assetPath = "%2e%2e/json/version";
console.log("raw guard catches?", assetPath.split("/").some((seg) => seg === ".."));
console.log(new URL(`https://chrome-devtools-frontend.appspot.com/serve_rev/@rev/${assetPath}`).pathname);
NODERepository: EvanBacon/serve-sim
Length of output: 206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant middleware section and nearby helpers/constants.
file="packages/serve-sim/src/middleware.ts"
printf '\n== File size ==\n'
wc -l "$file"
printf '\n== Relevant excerpt ==\n'
sed -n '1220,1295p' "$file"
printf '\n== Search for devtools asset path handling ==\n'
rg -n "devtoolsFrontendBase|DEVTOOLS_FRONTEND_REV|assetPath|decodeURIComponent|new URL\\(" "$file"Repository: EvanBacon/serve-sim
Length of output: 5159
Decode the asset path before proxying DevTools assets.
%2e%2e bypasses the raw split("/") check, and fetch(new URL(...)) normalizes it into a parent-path escape, so this can leave the intended /serve_rev/@REV/ subtree. Reject decoded segments before building the upstream URL.
🤖 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 `@packages/serve-sim/src/middleware.ts` around lines 1254 - 1266, The DevTools
asset proxy in middleware.ts is validating the raw assetPath, but encoded
traversal like %2e%2e can still escape when the upstream URL is built. Update
the assetPath handling in the middleware flow to decode and validate path
segments before the fetch call, and reject any decoded “..” segments in the same
place where the current split("/") check happens. Use the existing assetPath,
devtoolsFrontendBase, and fetch logic to keep the fix localized.
Comment on lines
+1637
to
+1643
| let lastSent = await computeConfig(); | ||
| res.write("data: " + lastSent + "\n\n"); | ||
|
|
||
| let closed = false; | ||
| const sendIfChanged = () => { | ||
| const sendIfChanged = async () => { | ||
| if (closed || res.writableEnded) return; | ||
| const next = computeConfig(); | ||
| const next = await computeConfig(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Install the close guard before the initial async recompute.
Line 1637 can wait on readServeSimStates(). If the client disconnects during that await, the close listener is not registered yet, so the handler can later start the watcher/heartbeat with no cleanup path.
Proposed fix
- let lastSent = await computeConfig();
+ let closed = false;
+ req.on("close", () => {
+ closed = true;
+ });
+
+ let lastSent = await computeConfig();
+ if (closed || res.writableEnded) return;
res.write("data: " + lastSent + "\n\n");
- let closed = false;
const sendIfChanged = async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let lastSent = await computeConfig(); | |
| res.write("data: " + lastSent + "\n\n"); | |
| let closed = false; | |
| const sendIfChanged = () => { | |
| const sendIfChanged = async () => { | |
| if (closed || res.writableEnded) return; | |
| const next = computeConfig(); | |
| const next = await computeConfig(); | |
| let closed = false; | |
| req.on("close", () => { | |
| closed = true; | |
| }); | |
| let lastSent = await computeConfig(); | |
| if (closed || res.writableEnded) return; | |
| res.write("data: " + lastSent + "\n\n"); | |
| const sendIfChanged = async () => { | |
| if (closed || res.writableEnded) return; | |
| const next = await computeConfig(); |
🤖 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 `@packages/serve-sim/src/middleware.ts` around lines 1637 - 1643, The SSE
handler in middleware.ts sets up the closed cleanup too late, after the initial
await on computeConfig/readServeSimStates, so a disconnect during that wait can
leak the watcher/heartbeat. Move the closed flag and close listener registration
before the first async recompute in the SSE setup block, and make sure the
existing sendIfChanged/res connection cleanup logic uses that early guard so the
stream can always be torn down correctly.
kabiroberai
force-pushed
the
perf-improvements
branch
2 times, most recently
from
July 1, 2026 03:38
5aeb057 to
d7b1e7a
Compare
kabiroberai
force-pushed
the
perf-improvements
branch
from
July 1, 2026 03:53
d7b1e7a to
923817e
Compare
Contributor
Author
|
Test failures seem to be flakes, they pass locally for me |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improve performance by async-ifying all the things
Summary by CodeRabbit
next()and enabled linked sourcemaps.