Refine simulator panel and inspect mode#261
Merged
Merged
Conversation
# Conflicts: # apps/web/src/features/simulator/ui/SimulatorPanel.tsx
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds a native siminspector (dylib + Objective‑C socket server), backend inspector APIs and UDID/port coordination, frontend inspector UI and types, device-frame refactor removing device‑chrome, dual installed-app manifest resolution, new simulator commands, and packaging/build updates across backend, web, and device‑use packages. ChangesInspector & Inspection UI
Device Frame, Device‑Chrome Removal & Installed‑App Resolution
Sequence DiagramsequenceDiagram
actor User
participant Frontend as Frontend (React)
participant Backend as Backend (Node)
participant Simulator as iOS Simulator
participant Inspector as siminspector Socket
User->>Frontend: Toggle Inspect mode
Frontend->>Backend: sim:inspectStart(workspaceId, bundleId?)
Backend->>Simulator: xcrun simctl launch with DYLD_INSERT_LIBRARIES=siminspector
Simulator->>Inspector: Load dylib → open AF_UNIX socket
Backend->>Inspector: Connect and send {"command":"snapshot"}
Inspector->>Simulator: Traverse UIView/CALayer tree on main thread
Inspector-->>Backend: JSON InspectorSnapshot\n
Backend-->>Frontend: sim:inspectSnapshot payload
Frontend->>Frontend: Parse snapshot, flatten nodes, compute bounds
User->>Frontend: Alt+click on element
Frontend->>Frontend: Pick node, show InspectorDetailsPanel, "Add to Chat"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 4
🧹 Nitpick comments (7)
apps/web/src/features/simulator/ui/SimulatorDeviceHeader.tsx (1)
130-145: ⚡ Quick winAdd exhaustiveness check to
getStatusLabel.The switch statement lacks a
defaultcase or exhaustiveness guard. If a new phase is added toSimPhase, this function would silently returnundefined. Consider adding adefaultwith aneverassertion or usingts-patternwith.exhaustive().Proposed fix
function getStatusLabel(state: SimPhase) { switch (state.phase) { case "idle": return "Simulator idle"; case "booting": return "Simulator booting"; case "streaming": return "Simulator streaming"; case "building": return "Building app"; case "running": return "App running"; case "error": return "Simulator error"; + default: { + const _exhaustive: never = state; + return _exhaustive; + } } }🤖 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 `@apps/web/src/features/simulator/ui/SimulatorDeviceHeader.tsx` around lines 130 - 145, getStatusLabel lacks an exhaustiveness check so adding new variants to SimPhase can return undefined; update getStatusLabel to include a default branch that asserts the unreachable value (use a never-typed helper, e.g. function assertUnreachable(x: never): never) and call it in the default case, or replace the switch with a pattern-matching approach (ts-pattern/.exhaustive()) to force compile-time exhaustiveness for the SimPhase union.apps/web/src/features/simulator/machine.ts (1)
62-146: 💤 Low valueConsider migrating to
ts-patternfor exhaustiveness checking.The state machine uses a native
switchwith a manualneverexhaustiveness check. Per coding guidelines,ts-patternwith.exhaustive()is preferred for discriminated unions. This is existing code structure, so migration can be deferred.🤖 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 `@apps/web/src/features/simulator/machine.ts` around lines 62 - 146, The current transition function uses a switch on event with a manual never-check (_exhaustive) for exhaustiveness; replace it with ts-pattern's match to get .exhaustive() checking: install/import match from 'ts-pattern', convert the switch in transition(current: SimPhase, event: SimEvent) into a match(event).with({ type: "BOOT" }, e => ... ) chain covering all event.type cases (BOOT, SWITCH_DEVICE, STREAM_READY, BUILD_START, BUILD_SUCCESS, APP_UNINSTALLED, STOP, ERROR, CLEAR) and return the same SimPhase results (use current.udid/current.stream where needed) and finish with .exhaustive(), removing the manual _exhaustive variable; ensure return types remain SimPhase | null and preserve all existing guards (e.g., current.phase checks) inside each handler.apps/web/src/features/simulator/ui/SimulatorPanel.tsx (2)
783-882: ⚖️ Poor tradeoffConsider extracting
InspectorDetailsPanelto a separate file.As a nested function component, it's recreated on every
SimulatorPanelrender. While the current implementation works, extracting it would:
- Improve code organization (this file is now ~986 lines)
- Allow React.memo optimization if needed later
- Follow the pattern of other extracted components (
SimulatorDeviceHeader,SimulatorStreamViewer, etc.)🤖 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 `@apps/web/src/features/simulator/ui/SimulatorPanel.tsx` around lines 783 - 882, InspectorDetailsPanel is defined as a nested function component inside SimulatorPanel which causes it to be recreated on every render; extract it into its own file by creating a new component file (e.g., InspectorDetailsPanel.tsx) that exports the InspectorDetailsPanel component (accepting props: node: InspectorNode, prompt: string, onPromptChange: (value: string) => void, onClose: () => void, onSendToChat: () => void), move the JSX and prop typings there, import any types it needs (InspectorNode) from the same module as SimulatorPanel, replace the nested function in SimulatorPanel with a simple import/use of InspectorDetailsPanel, and optionally wrap the exported component in React.memo to allow future memoization and match the project's component extraction pattern.
601-607: 💤 Low valueConsider adding a stale-closure guard for the polling interval.
The interval callback captures
refreshInspectorSnapshotwhich depends onworkspaceId. If the workspace changes while inspect mode is active, the interval could fetch data for the wrong workspace before the cleanup runs.A generation counter pattern (like
workspaceGenerationRefalready used elsewhere in this file) would prevent stale updates:🛡️ Suggested guard pattern
useEffect(() => { if (!inspectMode) return; + const gen = workspaceGenerationRef.current; const timer = setInterval(() => { - refreshInspectorSnapshot().catch((err) => setInspectError(getErrorMessage(err))); + refreshInspectorSnapshot() + .then((snapshot) => { + if (workspaceGenerationRef.current !== gen) return; + // snapshot already set inside refreshInspectorSnapshot + }) + .catch((err) => { + if (workspaceGenerationRef.current !== gen) return; + setInspectError(getErrorMessage(err)); + }); }, 1000); return () => clearInterval(timer); }, [inspectMode, refreshInspectorSnapshot]);🤖 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 `@apps/web/src/features/simulator/ui/SimulatorPanel.tsx` around lines 601 - 607, The polling effect for inspect mode can call refreshInspectorSnapshot for a stale workspace; capture the current workspace generation from the existing workspaceGenerationRef when installing the interval and skip ticks whose generation no longer matches so the callback never invokes refreshInspectorSnapshot for an out-of-date workspace. Concretely: inside the useEffect that depends on inspectMode and refreshInspectorSnapshot, read const generationAtStart = workspaceGenerationRef.current when creating the timer and in the interval callback first check if (generationAtStart !== workspaceGenerationRef.current) return; before calling refreshInspectorSnapshot().catch(...); this uses the existing workspaceGenerationRef to guard refreshInspectorSnapshot and prevent stale-closure polling when workspace changes.apps/web/src/features/simulator/api/simulator.service.ts (1)
55-61: ⚡ Quick win
parseInspectorSnapshotvalidates structure presence but not shape.The function checks that
snapshotexists and is an object, but then casts directly toInspectorSnapshotwithout validating that the object actually conforms to that interface (e.g., hasroots,bundleId,pid,timestamp). If the backend returns an object with missing or wrong-typed fields, consumers will encounter runtime errors when accessing properties likesnapshot.roots.Consider either:
- Adding minimal shape validation before the cast
- Using a schema validator (e.g., Zod) for the response
Example with minimal validation
function parseInspectorSnapshot(result: unknown): InspectorSnapshot { const snapshot = asRecord(result).snapshot; if (!snapshot || typeof snapshot !== "object") { throw new Error("Malformed inspector snapshot response"); } - return snapshot as InspectorSnapshot; + const s = snapshot as Record<string, unknown>; + if (!Array.isArray(s.roots) || typeof s.bundleId !== "string") { + throw new Error("Malformed inspector snapshot response"); + } + return snapshot as InspectorSnapshot; }🤖 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 `@apps/web/src/features/simulator/api/simulator.service.ts` around lines 55 - 61, The parseInspectorSnapshot function only checks snapshot existence but then force-casts to InspectorSnapshot; add minimal runtime shape validation in parseInspectorSnapshot to verify required fields (e.g., roots is an array, bundleId is a string, pid is a number, timestamp is a number) before returning, and throw a descriptive error if any field is missing or wrong-typed; alternatively replace the manual checks by validating the result with a schema validator (e.g., a Zod schema for InspectorSnapshot) and use that to parse/return the typed value.apps/web/src/features/simulator/ui/SimulatorStreamViewer.tsx (1)
387-407: 💤 Low valueInspect mode hover fires even during drag.
When
inspectModeis true and the user is dragging (buttons === 1) without Alt, the handler first firesonInspectorHoverthen continues to send touch events. This causes unnecessary hover callbacks during normal drag operations. Consider guarding the hover call:Suggested fix
const handleMouseMove = useCallback( (e: React.MouseEvent) => { - if (inspectMode) { + if (inspectMode && e.buttons === 0) { onInspectorHover?.(getInspectorNode(e)); } if (inspectMode && e.altKey) return; if (!isLive || e.buttons !== 1) return;🤖 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 `@apps/web/src/features/simulator/ui/SimulatorStreamViewer.tsx` around lines 387 - 407, The handler handleMouseMove currently calls onInspectorHover(getInspectorNode(e)) whenever inspectMode is true even if the user is actively dragging (e.buttons === 1) and not holding Alt; change the logic so that when inspectMode is true you first check the drag/Alt guard (i.e., if inspectMode && e.altKey is false and e.buttons === 1 then treat as a drag and do not call onInspectorHover), and only call onInspectorHover(getInspectorNode(e)) when inspectMode is true and the event is not a drag (or Alt is pressed). Update the conditional ordering inside handleMouseMove (using getInspectorNode, onInspectorHover, inspectMode, e.buttons, e.altKey) so hover callbacks are skipped during normal drag operations.apps/backend/src/services/simulator-context.ts (1)
573-668: 💤 Low valuePort reservation released even when stream reuses existing UDID stream.
At line 607,
releasePortReservation(reservedPort)is called afterspawnStreamsucceeds, andreservedPortis set to null. However, ifspawnStreamreturns an existing stream (lines 365-372), the reserved port was never actually used by the new stream—it just returns the existing port. The reservation is still released correctly, which is fine, but the port was reserved unnecessarily.This is a minor inefficiency; the port scan work is wasted when reusing an existing stream.
Consider checking for existing stream before reserving
+ // Check if this UDID already has a stream before reserving a port + const existing = activeStreams.get(udid); + if (existing) { + try { + process.kill(existing.pid, 0); + // Reuse existing stream, skip port reservation + // ... handle reuse case + } catch { + activeStreams.delete(udid); + } + } + reservedPort = await reservePort(3100 + sessions.size + startingUdids.size);🤖 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 `@apps/backend/src/services/simulator-context.ts` around lines 573 - 668, Before reserving a port, check whether an active stream already exists for the UDID and only call reservePort/reservePortReservation when you truly need to spawn a new stream; specifically, change the sequence around spawnStream/reservePort so that you either (a) call a new helper like getActiveStreamPort(udid) (or extend spawnStream to report reuse) and, if it returns a port, skip reservePort and use that port, or (b) call spawnStream with an explicit optionalPort parameter and only call reservePort when spawnStream indicates it will create a new stream; update the code that currently sets reservedPort, calls reservePort(…), then spawnStream(udid, reservedPort) to first detect existing stream, and only set/release reservedPort when you actually created a new stream (ensure releasePortReservation(reservedPort) and reservedPort = null remain correct when creation succeeds).
🤖 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 `@apps/web/src/features/simulator/ui/SimulatorContentHeader.tsx`:
- Around line 55-57: In the SimulatorContentHeader component update the
simulator picker trigger element (the JSX element with aria-label="Select
simulator device") to use correct menu semantics by changing
aria-haspopup="listbox" to aria-haspopup="menu" (or removing the attribute to
let default semantics apply); ensure this change is applied on the dropdown
trigger JSX so it matches the DropdownMenu usage and assistive tech sees a menu
popup.
In `@apps/web/src/features/simulator/ui/SimulatorLaunchPreview.tsx`:
- Around line 13-16: The error branch in SimulatorLaunchPreview renders an alert
even when errorMessage is undefined, causing a blank destructive state; update
the render logic in SimulatorLaunchPreview so the error alert is only rendered
when errorMessage is truthy (and only show the retry action when both canRetry
and onRetry are provided), i.e., gate the alert on errorMessage and gate the
retry button on canRetry && typeof onRetry === 'function' while preserving
existing props (errorMessage, canRetry, onRetry, onStart).
In `@packages/device-use/native/Sources/SimInspector/Inspector.m`:
- Around line 228-229: The current call to write(clientFd, output.bytes,
output.length) may perform a short write and truncate large json_line(response)
payloads; replace it with a loop that repeatedly writes the remaining bytes from
json_line(response) until total bytes == output.length, handling partial writes
by advancing the buffer pointer and reducing the remaining count, and on error
handle EINTR by retrying and treat other errors as fatal. Locate the write site
in Inspector.m (the json_line(response) / write(clientFd, ...) usage) and
implement this retry/partial-write logic (optionally using send(clientFd, ptr,
remaining, MSG_NOSIGNAL) if preferred).
In `@packages/device-use/scripts/build-native.ts`:
- Around line 57-64: The check for siminspector currently only uses
existsSync(inspectorBinary) (inspectorReady) which permits stale single-arch
dylibs to skip rebuilding; update the readiness check to mirror the simbridge
validation by verifying the inspector binary has both architectures (e.g., run
the same arch validation logic used for releaseBinary or a lipo/file check) and
treat inspectorReady as false if the universal/expected archs are not present so
the build path (build.sh) will rebuild a universal binary for both
electron-builder.yml targets; update the conditional that uses bridgeReady and
inspectorReady and the variables inspectorBinary/releaseBinary accordingly so
packaging never skips when inspectorBinary is single-arch or missing.
---
Nitpick comments:
In `@apps/backend/src/services/simulator-context.ts`:
- Around line 573-668: Before reserving a port, check whether an active stream
already exists for the UDID and only call reservePort/reservePortReservation
when you truly need to spawn a new stream; specifically, change the sequence
around spawnStream/reservePort so that you either (a) call a new helper like
getActiveStreamPort(udid) (or extend spawnStream to report reuse) and, if it
returns a port, skip reservePort and use that port, or (b) call spawnStream with
an explicit optionalPort parameter and only call reservePort when spawnStream
indicates it will create a new stream; update the code that currently sets
reservedPort, calls reservePort(…), then spawnStream(udid, reservedPort) to
first detect existing stream, and only set/release reservedPort when you
actually created a new stream (ensure releasePortReservation(reservedPort) and
reservedPort = null remain correct when creation succeeds).
In `@apps/web/src/features/simulator/api/simulator.service.ts`:
- Around line 55-61: The parseInspectorSnapshot function only checks snapshot
existence but then force-casts to InspectorSnapshot; add minimal runtime shape
validation in parseInspectorSnapshot to verify required fields (e.g., roots is
an array, bundleId is a string, pid is a number, timestamp is a number) before
returning, and throw a descriptive error if any field is missing or wrong-typed;
alternatively replace the manual checks by validating the result with a schema
validator (e.g., a Zod schema for InspectorSnapshot) and use that to
parse/return the typed value.
In `@apps/web/src/features/simulator/machine.ts`:
- Around line 62-146: The current transition function uses a switch on event
with a manual never-check (_exhaustive) for exhaustiveness; replace it with
ts-pattern's match to get .exhaustive() checking: install/import match from
'ts-pattern', convert the switch in transition(current: SimPhase, event:
SimEvent) into a match(event).with({ type: "BOOT" }, e => ... ) chain covering
all event.type cases (BOOT, SWITCH_DEVICE, STREAM_READY, BUILD_START,
BUILD_SUCCESS, APP_UNINSTALLED, STOP, ERROR, CLEAR) and return the same SimPhase
results (use current.udid/current.stream where needed) and finish with
.exhaustive(), removing the manual _exhaustive variable; ensure return types
remain SimPhase | null and preserve all existing guards (e.g., current.phase
checks) inside each handler.
In `@apps/web/src/features/simulator/ui/SimulatorDeviceHeader.tsx`:
- Around line 130-145: getStatusLabel lacks an exhaustiveness check so adding
new variants to SimPhase can return undefined; update getStatusLabel to include
a default branch that asserts the unreachable value (use a never-typed helper,
e.g. function assertUnreachable(x: never): never) and call it in the default
case, or replace the switch with a pattern-matching approach
(ts-pattern/.exhaustive()) to force compile-time exhaustiveness for the SimPhase
union.
In `@apps/web/src/features/simulator/ui/SimulatorPanel.tsx`:
- Around line 783-882: InspectorDetailsPanel is defined as a nested function
component inside SimulatorPanel which causes it to be recreated on every render;
extract it into its own file by creating a new component file (e.g.,
InspectorDetailsPanel.tsx) that exports the InspectorDetailsPanel component
(accepting props: node: InspectorNode, prompt: string, onPromptChange: (value:
string) => void, onClose: () => void, onSendToChat: () => void), move the JSX
and prop typings there, import any types it needs (InspectorNode) from the same
module as SimulatorPanel, replace the nested function in SimulatorPanel with a
simple import/use of InspectorDetailsPanel, and optionally wrap the exported
component in React.memo to allow future memoization and match the project's
component extraction pattern.
- Around line 601-607: The polling effect for inspect mode can call
refreshInspectorSnapshot for a stale workspace; capture the current workspace
generation from the existing workspaceGenerationRef when installing the interval
and skip ticks whose generation no longer matches so the callback never invokes
refreshInspectorSnapshot for an out-of-date workspace. Concretely: inside the
useEffect that depends on inspectMode and refreshInspectorSnapshot, read const
generationAtStart = workspaceGenerationRef.current when creating the timer and
in the interval callback first check if (generationAtStart !==
workspaceGenerationRef.current) return; before calling
refreshInspectorSnapshot().catch(...); this uses the existing
workspaceGenerationRef to guard refreshInspectorSnapshot and prevent
stale-closure polling when workspace changes.
In `@apps/web/src/features/simulator/ui/SimulatorStreamViewer.tsx`:
- Around line 387-407: The handler handleMouseMove currently calls
onInspectorHover(getInspectorNode(e)) whenever inspectMode is true even if the
user is actively dragging (e.buttons === 1) and not holding Alt; change the
logic so that when inspectMode is true you first check the drag/Alt guard (i.e.,
if inspectMode && e.altKey is false and e.buttons === 1 then treat as a drag and
do not call onInspectorHover), and only call
onInspectorHover(getInspectorNode(e)) when inspectMode is true and the event is
not a drag (or Alt is pressed). Update the conditional ordering inside
handleMouseMove (using getInspectorNode, onInspectorHover, inspectMode,
e.buttons, e.altKey) so hover callbacks are skipped during normal drag
operations.
🪄 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
Run ID: d32e77aa-3c56-4b1d-b6af-71dc9590b27a
📒 Files selected for processing (32)
apps/backend/src/config/installed-apps.tsapps/backend/src/services/agent/commands.tsapps/backend/src/services/simulator-context.tsapps/backend/test/unit/shared/events.test.tsapps/web/src/features/simulator/api/simulator.service.tsapps/web/src/features/simulator/device-chrome.tsapps/web/src/features/simulator/machine.tsapps/web/src/features/simulator/store.tsapps/web/src/features/simulator/types.tsapps/web/src/features/simulator/ui/AppleLogoIcon.tsxapps/web/src/features/simulator/ui/DeviceFrame.tsxapps/web/src/features/simulator/ui/SimulatorContentHeader.tsxapps/web/src/features/simulator/ui/SimulatorDeviceHeader.tsxapps/web/src/features/simulator/ui/SimulatorEmptySurface.tsxapps/web/src/features/simulator/ui/SimulatorLaunchPreview.tsxapps/web/src/features/simulator/ui/SimulatorPanel.tsxapps/web/src/features/simulator/ui/SimulatorStreamViewer.tsxapps/web/src/features/simulator/ui/simulatorDisplay.tselectron-builder.ymlpackage.jsonpackages/device-use/native/Sources/SimInspector/Inspector.mpackages/device-use/native/Sources/SimInspector/build.shpackages/device-use/package.jsonpackages/device-use/scripts/build-native.tspackages/device-use/scripts/compile-bin.tspackages/device-use/src/cli/commands/serve.tspackages/device-use/src/engine/index.tspackages/device-use/src/engine/simbridge.tsscripts/prepare-device-chrome.mjsscripts/prepare-device-use.mjsshared/events.tstest/unit/simulator/machine.test.ts
💤 Files with no reviewable changes (3)
- apps/web/src/features/simulator/device-chrome.ts
- package.json
- scripts/prepare-device-chrome.mjs
Owner
Author
Greenlight — Round 1Fixed (4): simulator picker ARIA menu semantics, launch error fallback copy, complete inspector socket writes, and universal siminspector architecture validation. Local validation passed: |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/web/src/features/simulator/ui/SimulatorLaunchPreview.tsx (1)
74-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard retry button on a valid callback, not just
canRetry.If
canRetryis true whileonRetryis missing, the button renders but does nothing.✅ Minimal fix
- {canRetry && ( + {canRetry && typeof onRetry === "function" && ( <Button variant="outline" onClick={onRetry} className="min-h-10 min-w-[136px] gap-2 rounded-xl" >🤖 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 `@apps/web/src/features/simulator/ui/SimulatorLaunchPreview.tsx` around lines 74 - 83, The retry button currently renders when canRetry is true even if onRetry is undefined; update the render guard in SimulatorLaunchPreview (the JSX that includes Button, RotateCcw and "Try Again") to require both canRetry and a valid callback (e.g., check onRetry is a function or truthy) before rendering, or alternatively render the Button disabled when onRetry is absent; ensure you reference the existing onClick={onRetry} handler so the button is only interactive when onRetry is provided.
🧹 Nitpick comments (1)
apps/web/src/features/simulator/ui/SimulatorLaunchPreview.tsx (1)
50-85: ⚡ Quick winUse
ts-pattern+.exhaustive()forphaserendering.This phase union is currently rendered with ad-hoc conditional branches; switching to
ts-patterngives enforced exhaustiveness and prevents silent misses when a new phase is added.♻️ Suggested refactor
+import { match } from "ts-pattern"; ... - {phase === "idle" && ( - <Button - onClick={onStart} - disabled={!selectedUdid} - className="min-h-11 min-w-[180px] gap-2 rounded-xl transition-[background-color,border-color,color,box-shadow] duration-150" - > - <Play className="h-4 w-4" /> - Start Simulator - </Button> - )} - - {phase === "booting" && ( - <div className="flex flex-col items-center gap-3" aria-live="polite"> - <Loader2 className="text-primary h-6 w-6 animate-spin" /> - <p className="text-text-secondary text-sm font-medium">Starting simulator</p> - </div> - )} - - {phase === "error" && ( - <div className="flex max-w-[240px] flex-col items-center gap-3" aria-live="polite"> - <AlertCircle className="text-destructive h-5 w-5" /> - <p className="text-destructive text-sm leading-5"> - {errorMessage ?? "Something went wrong starting the simulator."} - </p> - {canRetry && ( - <Button - variant="outline" - onClick={onRetry} - className="min-h-10 min-w-[136px] gap-2 rounded-xl" - > - <RotateCcw className="h-4 w-4" /> - Try Again - </Button> - )} - </div> - )} + {match(phase) + .with("idle", () => ( + <Button + onClick={onStart} + disabled={!selectedUdid || typeof onStart !== "function"} + className="min-h-11 min-w-[180px] gap-2 rounded-xl transition-[background-color,border-color,color,box-shadow] duration-150" + > + <Play className="h-4 w-4" /> + Start Simulator + </Button> + )) + .with("booting", () => ( + <div className="flex flex-col items-center gap-3" aria-live="polite"> + <Loader2 className="text-primary h-6 w-6 animate-spin" /> + <p className="text-text-secondary text-sm font-medium">Starting simulator</p> + </div> + )) + .with("error", () => ( + <div className="flex max-w-[240px] flex-col items-center gap-3" aria-live="polite"> + <AlertCircle className="text-destructive h-5 w-5" /> + <p className="text-destructive text-sm leading-5"> + {errorMessage ?? "Something went wrong starting the simulator."} + </p> + {canRetry && typeof onRetry === "function" && ( + <Button + variant="outline" + onClick={onRetry} + className="min-h-10 min-w-[136px] gap-2 rounded-xl" + > + <RotateCcw className="h-4 w-4" /> + Try Again + </Button> + )} + </div> + )) + .exhaustive()}As per coding guidelines,
**/*.{ts,tsx}: Prefer ts-pattern for discriminated unions. Use .exhaustive() for exhaustiveness checking.🤖 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 `@apps/web/src/features/simulator/ui/SimulatorLaunchPreview.tsx` around lines 50 - 85, Replace the ad-hoc conditional JSX branches for the local "phase" union in SimulatorLaunchPreview with a ts-pattern match to enforce exhaustiveness: import { match } from 'ts-pattern', then wrap the JSX return for the three cases by calling match(phase).with('idle', () => /* return the Start Button JSX using selectedUdid and onStart */).with('booting', () => /* return Loader JSX */).with('error', () => /* return Error JSX using errorMessage, canRetry and onRetry */).exhaustive(); ensure all current props/variables (selectedUdid, onStart, errorMessage, canRetry, onRetry) are used inside the corresponding arms and replace the existing conditional blocks with this single match expression.
🤖 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 `@apps/web/src/features/simulator/ui/SimulatorLaunchPreview.tsx`:
- Around line 51-54: The Start button currently binds onStart directly even
though onStart is optional, allowing a clickable button with no effect; update
the Button in SimulatorLaunchPreview (the element using props onStart and
selectedUdid) to guard against undefined onStart by disabling the button when
onStart is not provided or by wiring a safe caller (e.g., onClick={() =>
onStart?.()}) so clicks become no-ops only when appropriate; ensure the disabled
prop reflects both !selectedUdid and !onStart so the button is not interactable
when there is no handler.
In `@packages/device-use/native/Sources/SimInspector/Inspector.m`:
- Around line 208-221: Check that the request[@"command"] is an NSString before
calling isEqualToString: to avoid unrecognized selector crashes; replace the
current direct use of NSString *command = request[@"command"] and subsequent if
([command isEqualToString:@"..."]) checks with a guarded variant that verifies
[command isKindOfClass:[NSString class]] and returns @{@"ok": `@NO`, @"error":
@"invalid command type"} (or similar) when it's not a string, keeping the
existing handling for @"ping" and @"snapshot" (which calls snapshot_payload())
intact.
- Around line 258-273: The code currently unlinks addr.sun_path unconditionally
which can remove a live inspector socket and cause ownership races; update the
logic in Inspector.m around socket_path(), unlink, bind/listen so that before
unlinking you probe the existing path by creating a temporary AF_UNIX
SOCK_STREAM client socket and attempting to connect to addr.sun_path: if connect
succeeds (i.e., another live owner) do not unlink and abort/return, if connect
fails with ECONNREFUSED or ENOENT proceed to unlink and continue with bind, and
ensure you close the temporary probe socket and check errno for the correct
failure cases (use the same sockaddr_un structure and path.UTF8String for the
probe) so you only remove stale sockets and avoid clobbering live ones.
---
Duplicate comments:
In `@apps/web/src/features/simulator/ui/SimulatorLaunchPreview.tsx`:
- Around line 74-83: The retry button currently renders when canRetry is true
even if onRetry is undefined; update the render guard in SimulatorLaunchPreview
(the JSX that includes Button, RotateCcw and "Try Again") to require both
canRetry and a valid callback (e.g., check onRetry is a function or truthy)
before rendering, or alternatively render the Button disabled when onRetry is
absent; ensure you reference the existing onClick={onRetry} handler so the
button is only interactive when onRetry is provided.
---
Nitpick comments:
In `@apps/web/src/features/simulator/ui/SimulatorLaunchPreview.tsx`:
- Around line 50-85: Replace the ad-hoc conditional JSX branches for the local
"phase" union in SimulatorLaunchPreview with a ts-pattern match to enforce
exhaustiveness: import { match } from 'ts-pattern', then wrap the JSX return for
the three cases by calling match(phase).with('idle', () => /* return the Start
Button JSX using selectedUdid and onStart */).with('booting', () => /* return
Loader JSX */).with('error', () => /* return Error JSX using errorMessage,
canRetry and onRetry */).exhaustive(); ensure all current props/variables
(selectedUdid, onStart, errorMessage, canRetry, onRetry) are used inside the
corresponding arms and replace the existing conditional blocks with this single
match expression.
🪄 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
Run ID: 7b267b05-5949-431b-a936-7498a359bd41
📒 Files selected for processing (4)
apps/web/src/features/simulator/ui/SimulatorContentHeader.tsxapps/web/src/features/simulator/ui/SimulatorLaunchPreview.tsxpackages/device-use/native/Sources/SimInspector/Inspector.mpackages/device-use/scripts/build-native.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/features/simulator/ui/SimulatorContentHeader.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/device-use/scripts/build-native.ts
Owner
Author
Greenlight — Round 2Fixed (3): disabled no-op Start button states, validated inspector command type, and avoided unlinking live inspector sockets. Local validation passed: |
This was referenced May 10, 2026
This was referenced May 18, 2026
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
1 participant
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.
Refactors the simulator panel into focused header/device/empty-state components and simplifies the idle/running UI with a cleaner picker, device header controls, and simplified frame rendering. Adds simulator switching/ownership safeguards, stream lifecycle cleanup, safer event parsing, packaged device-use manifest/resource resolution, and universal native helper preparation. Extends device-use inspector/simbridge support including redacted secure text snapshots and packaged siminspector handling, while removing the old device-chrome generation path. Validated with
bun run typecheck,bun run typecheck:backend,bun run --cwd packages/device-use typecheck, andbunx vitest run --config test/vitest.config.ts test/unit/simulator/machine.test.ts.Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Removals
Chores