feat: add event log#124
Conversation
📝 WalkthroughWalkthroughThis PR adds a simulator event log across server, CLI, and client paths, with in-memory storage, HID/command capture, JSON/SSE endpoints, an ChangesSimulator event log feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/serve-sim/src/device-session.ts (2)
67-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate of
formatPoint/formatNumberfromevent-log.ts.
formatEventLogPoint/formatEventLogNumberare copy-pasted, byte-for-byte, from the privateformatPoint/formatNumberhelpers inevent-log.ts. Exporting those fromevent-log.ts(or a shared util) and importing here would avoid drift between the two implementations as either evolves.🤖 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/device-session.ts` around lines 67 - 74, The helpers formatEventLogPoint and formatEventLogNumber in device-session.ts are duplicated from the private formatPoint and formatNumber logic in event-log.ts; replace the copy-paste with a shared source of truth by exporting those helpers from event-log.ts or moving them into a common utility and importing them here. Keep the behavior identical, but ensure device-session.ts uses the shared formatPoint/formatNumber symbols so the implementations cannot drift.
400-480: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNo test coverage for the tap/drag gesture coalescing logic.
recordTouchEventhas several stateful branches (begin/move/end, threshold-based tap-vs-drag classification, create-vs-update of the log entry). Given its complexity and that it's the primary HID capture path for the new feature, it would benefit from unit tests (mockingrecordEventLogEvent/updateEventLogEventor the module) before merge.🤖 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/device-session.ts` around lines 400 - 480, Add unit tests for recordTouchEvent in device-session.ts to cover the stateful begin/move/end flow and the tap-vs-drag threshold behavior. Mock recordEventLogEvent and updateEventLogEvent (or the event-log module) and verify that a begin initializes touchGestureLog, move coalesces into a drag and switches from create to update when eventId exists, and end emits either a tap or drag based on touchGestureMoved. Use the recordTouchEvent method and touchGestureLog-related helpers as anchors when locating the logic.packages/serve-sim/src/__tests__/event-log.test.ts (1)
16-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test for the 500-entry eviction cap.
The suite covers ordering, filtering, since/limit reads, subscriptions, and command/HID mapping well, but doesn't exercise
EVENT_LOG_MAX_ENTRIESeviction (i.e., recording >500 entries and verifying the oldest are dropped). This is a core invariant of the store worth pinning down.🤖 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/__tests__/event-log.test.ts` around lines 16 - 192, Add a test in event-log.test.ts to cover EVENT_LOG_MAX_ENTRIES eviction in the event log store. In the existing describe("event log store") suite, use recordEventLogEvent to create more than 500 entries, then verify readEventLog only returns the newest 500 and that the oldest IDs/entries are dropped while order is preserved. Reference the store APIs recordEventLogEvent and readEventLog so the behavior stays pinned down if implementation details change.packages/serve-sim/src/exec-ws.ts (1)
181-184: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilent catch around
onCommandResulthas no rationale/logging, unlike the analogous pattern inevent-log.ts.
notifyEventLogSubscribersinevent-log.tsdocuments why subscriber errors are swallowed. Here,onCommandResulterrors are also swallowed but without comment or any diagnostic trail, which could hide real bugs in the new event-log mapping logic during development.🔍 Add a comment / minimal diagnostic
try { opts.onCommandResult?.(command, result); - } catch {} + } catch { + // Event-log capture is a diagnostic side-channel; a failure here + // must not break the exec response path. + }🤖 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/exec-ws.ts` around lines 181 - 184, The try/catch around onCommandResult in exec-ws.ts is swallowing errors without any rationale or diagnostics. Update the onCommandResult callback handling in the command result path to match the documented pattern used by notifyEventLogSubscribers in event-log.ts: either add a brief comment explaining why subscriber errors are ignored, or log a minimal diagnostic before swallowing so failures in the event-log mapping are visible during development.packages/serve-sim/src/__tests__/exec-ws.test.ts (1)
191-199: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSSE retry only attempts once.
If the control channel emits more than one non-data frame before the actual
data:payload, this will fail since it only retries a single time. Consider looping with a small bound instead of a single conditional retry to reduce CI flakiness.♻️ Suggested loop-based retry
- let reply = await channel.next(); - let data = /^data: (.*)$/m.exec(reply.data ?? "")?.[1]; - if (!data) { - reply = await channel.next(); - data = /^data: (.*)$/m.exec(reply.data ?? "")?.[1]; - } + let reply = await channel.next(); + let data = /^data: (.*)$/m.exec(reply.data ?? "")?.[1]; + for (let attempts = 0; !data && attempts < 5; attempts++) { + reply = await channel.next(); + data = /^data: (.*)$/m.exec(reply.data ?? "")?.[1]; + }🤖 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/__tests__/exec-ws.test.ts` around lines 191 - 199, The SSE test in exec-ws.test.ts only retries once when parsing the control channel response, so it can still fail if multiple non-data frames arrive before the first data payload. Update the parsing logic around connect(), channel.next(), and the /^data: (.*)$/m extraction to use a small bounded loop instead of a single if retry, so it keeps advancing until it finds data or reaches the limit.packages/serve-sim/src/index.ts (1)
680-697: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant
execSyncshellouts per device.
getDeviceName(existing helper) re-runsxcrun simctl list devices -jand re-parses the full device list on every call. When events span multiple devices, this loop calls it once per unique device, synchronously shelling out repeatedly for data that's already available after the first call.♻️ Fetch the device list once and reuse it
+function listSimulatorDevices(): Record<string, Array<{ udid: string; name: string; state: string }>> { + try { + const output = execSync("xcrun simctl list devices -j", { encoding: "utf-8" }); + return (JSON.parse(output) as { devices: Record<string, Array<{ udid: string; name: string; state: string }>> }).devices; + } catch { + return {}; + } +} + function deviceLabelsForEvents(events: EventLogEntry[]): Map<string, string> { const devices = [...new Set(events.map((event) => event.device).filter((device): device is string => !!device))]; const names = new Map<string, string>(); + const allDevices = listSimulatorDevices(); for (const device of devices) { - names.set(device, getDeviceName(device) ?? device.slice(0, 8)); + const name = Object.values(allDevices).flat().find((d) => d.udid === device)?.name; + names.set(device, name ?? device.slice(0, 8)); }🤖 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/index.ts` around lines 680 - 697, The device label builder is repeatedly shelling out via getDeviceName for each unique device, which re-fetches and re-parses the full simctl device list multiple times. Refactor deviceLabelsForEvents to fetch the device list once and reuse it for all lookups, either by passing a cached lookup into getDeviceName or by computing the name map up front and then building labels from that single result. Keep the logic in deviceLabelsForEvents and the getDeviceName helper aligned so repeated calls do not invoke execSync per device.
🤖 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/event-log.ts`:
- Around line 210-223: The HID key event handling in event-log.ts is persisting
printable characters via keyLabelForUsage in the 0x06 case, which exposes typed
text in summary/details and downstream JSON/SSE/CLI output. Update the key
handling logic to avoid storing literal glyphs for printable usages: keep
special keys readable, but redact or generalize letters/numbers before
constructing the returned event object in the 0x06 case (and the matching
508-517 path) so the event log no longer contains sensitive keystrokes.
In `@packages/serve-sim/src/middleware.ts`:
- Around line 131-134: Wrap the event-log recording path in `recordCommandEvent`
so failures stay diagnostic-only and cannot break the main exec flow. Add
try/catch around the `eventLogEventForCommand` and `recordEventLogEvent` calls,
using the same defensive pattern as `notifyEventLogSubscribers`, and keep the
`/exec` completion handler and `exec-ws` `onCommandResult` callback insulated
from any throw coming out of `recordCommandEvent`.
- Around line 1257-1267: `handleUiRequest` has the same unguarded
event-recording gap for `ui-setting` as the exec path: after `setUiOption`
succeeds, the direct `recordEventLogEvent` call can throw and incorrectly fail
the whole request. Update the `ui-setting` branch in `handleUiRequest` to wrap
`recordEventLogEvent` in the same safe/error-swallowing pattern used for
exec-path recording so the request still returns `{ ok: true }` when the option
update succeeds.
---
Nitpick comments:
In `@packages/serve-sim/src/__tests__/event-log.test.ts`:
- Around line 16-192: Add a test in event-log.test.ts to cover
EVENT_LOG_MAX_ENTRIES eviction in the event log store. In the existing
describe("event log store") suite, use recordEventLogEvent to create more than
500 entries, then verify readEventLog only returns the newest 500 and that the
oldest IDs/entries are dropped while order is preserved. Reference the store
APIs recordEventLogEvent and readEventLog so the behavior stays pinned down if
implementation details change.
In `@packages/serve-sim/src/__tests__/exec-ws.test.ts`:
- Around line 191-199: The SSE test in exec-ws.test.ts only retries once when
parsing the control channel response, so it can still fail if multiple non-data
frames arrive before the first data payload. Update the parsing logic around
connect(), channel.next(), and the /^data: (.*)$/m extraction to use a small
bounded loop instead of a single if retry, so it keeps advancing until it finds
data or reaches the limit.
In `@packages/serve-sim/src/device-session.ts`:
- Around line 67-74: The helpers formatEventLogPoint and formatEventLogNumber in
device-session.ts are duplicated from the private formatPoint and formatNumber
logic in event-log.ts; replace the copy-paste with a shared source of truth by
exporting those helpers from event-log.ts or moving them into a common utility
and importing them here. Keep the behavior identical, but ensure
device-session.ts uses the shared formatPoint/formatNumber symbols so the
implementations cannot drift.
- Around line 400-480: Add unit tests for recordTouchEvent in device-session.ts
to cover the stateful begin/move/end flow and the tap-vs-drag threshold
behavior. Mock recordEventLogEvent and updateEventLogEvent (or the event-log
module) and verify that a begin initializes touchGestureLog, move coalesces into
a drag and switches from create to update when eventId exists, and end emits
either a tap or drag based on touchGestureMoved. Use the recordTouchEvent method
and touchGestureLog-related helpers as anchors when locating the logic.
In `@packages/serve-sim/src/exec-ws.ts`:
- Around line 181-184: The try/catch around onCommandResult in exec-ws.ts is
swallowing errors without any rationale or diagnostics. Update the
onCommandResult callback handling in the command result path to match the
documented pattern used by notifyEventLogSubscribers in event-log.ts: either add
a brief comment explaining why subscriber errors are ignored, or log a minimal
diagnostic before swallowing so failures in the event-log mapping are visible
during development.
In `@packages/serve-sim/src/index.ts`:
- Around line 680-697: The device label builder is repeatedly shelling out via
getDeviceName for each unique device, which re-fetches and re-parses the full
simctl device list multiple times. Refactor deviceLabelsForEvents to fetch the
device list once and reuse it for all lookups, either by passing a cached lookup
into getDeviceName or by computing the name map up front and then building
labels from that single result. Keep the logic in deviceLabelsForEvents and the
getDeviceName helper aligned so repeated calls do not invoke execSync per
device.
🪄 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: 12746ca3-42ff-4368-af94-fc90bdf65276
📒 Files selected for processing (15)
packages/serve-sim/README.mdpackages/serve-sim/src/__tests__/event-log-format.test.tspackages/serve-sim/src/__tests__/event-log.test.tspackages/serve-sim/src/__tests__/exec-ws.test.tspackages/serve-sim/src/__tests__/middleware-selection.test.tspackages/serve-sim/src/client/client.tsxpackages/serve-sim/src/client/components/event-log-tool.tsxpackages/serve-sim/src/client/components/tools-panel.tsxpackages/serve-sim/src/client/utils/sim-endpoint.tspackages/serve-sim/src/device-session.tspackages/serve-sim/src/event-log-format.tspackages/serve-sim/src/event-log.tspackages/serve-sim/src/exec-ws.tspackages/serve-sim/src/index.tspackages/serve-sim/src/middleware.ts
|
Thanks @coderabbitai, addressed the actionable feedback in acd9e26: printable key events now redact typed characters and omit raw printable HID usage; exec/UI-setting event-log recording is guarded so diagnostic logging cannot fail the primary request; eviction coverage, SSE retry hardening, shared point formatting, cached simulator labels, and the command-result side-channel rationale were also added. Validation: One note on the tap/drag coalescing test suggestion: I intentionally left direct |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/serve-sim/src/exec-ws.ts (1)
175-187: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winObserver receives internal
idbeyond documentedstdout/stderr/exitCodeshape.
resultincludesid(the request id) in addition tostdout/stderr/exitCode, and that whole object is forwarded toopts.onCommandResult. If the event-log observer persists this object as-is (e.g. via spread), the internal exec-channel request id leaks into stored/streamed event-log entries even though it has no semantic meaning there. Consider passing a trimmed object ({ stdout, stderr, exitCode }) to the observer instead of the fullresultused for the websocket response.🔧 Proposed fix
try { - opts.onCommandResult?.(command, result); + opts.onCommandResult?.(command, { + stdout: result.stdout, + stderr: result.stderr, + exitCode: result.exitCode, + }); } catch {🤖 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/exec-ws.ts` around lines 175 - 187, The exec command observer is receiving the full websocket response object, including the internal request identifier, instead of only the documented command result fields. Update the result handling in exec-ws.ts so execCommandResult/onCommandResult is called with a trimmed payload containing only stdout, stderr, and exitCode, while keeping the id only for send(result) on the websocket response path. Use the existing result construction and opts.onCommandResult callback in the exec flow to locate the change.
🤖 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.
Nitpick comments:
In `@packages/serve-sim/src/exec-ws.ts`:
- Around line 175-187: The exec command observer is receiving the full websocket
response object, including the internal request identifier, instead of only the
documented command result fields. Update the result handling in exec-ws.ts so
execCommandResult/onCommandResult is called with a trimmed payload containing
only stdout, stderr, and exitCode, while keeping the id only for send(result) on
the websocket response path. Use the existing result construction and
opts.onCommandResult callback in the exec flow to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 008edb20-6275-4f39-a1d4-d2408d39080c
📒 Files selected for processing (7)
packages/serve-sim/src/__tests__/event-log.test.tspackages/serve-sim/src/__tests__/exec-ws.test.tspackages/serve-sim/src/device-session.tspackages/serve-sim/src/event-log.tspackages/serve-sim/src/exec-ws.tspackages/serve-sim/src/index.tspackages/serve-sim/src/middleware.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/serve-sim/src/tests/event-log.test.ts
- packages/serve-sim/src/index.ts
- packages/serve-sim/src/tests/exec-ws.test.ts
- packages/serve-sim/src/middleware.ts
- packages/serve-sim/src/device-session.ts
- packages/serve-sim/src/event-log.ts
🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -1 acd9e26 2>/dev/null || echo "commit not found locally"
git show --stat acd9e26 2>/dev/null | head -50Length of output: 1005
Your rationale on skipping direct I'll take another pass over the diff to confirm the fixes land as described. ✅ Action performedReview finished.
|
1 participant
Why
serve-sim did not have one place to inspect the actions routed through the preview or CLI. When debugging agent-driven sessions, it is useful to see recent simulator actions in order: touches, gestures, hardware buttons, app installs, media drops, UI setting changes, screenshots, and related commands.
For EAS and other automation consumers, JSON events also need a stable message field. Adding a Bunyan-style
msgfield lets logs from serve-sim and other tools be gathered together without every consumer knowing serve-sim-specific display fields.How
DeviceSessioningress, plus recognized host-side simulator commands from/execand UI setting updates.msgto every JSON event entry while keepingsummaryas a compatibility/display alias.serve-sim event-log [-d udid] [--json] [--limit n]for CLI and automation consumers.Test Plan
bunx tsc --noEmitbun test packages/serve-sim/src/__tests__bun run packages/serve-sim/build.tsScreen.Recording.2026-07-02.at.17.27.46.mov
Summary by CodeRabbit
serve-sim event-logCLI subcommand to view recent events (formatted or JSON), with optional device filtering.serve-simREADME to reflect the new tool/CLI capabilities.