Ios Sim Fixes#221
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughExtends iOS simulator streaming with native IOSurface-Indigo capture and Indigo HID input helpers, introduces RPC tools for git user identity and listing open PRs, and adds a branch picker UI component for lane creation with commit metadata and PR filtering capabilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/components/lanes/LanesPage.tsx (1)
1536-1572:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset create-dialog identity/loading state before conditional fetches.
prepareCreateDialogclears branches/PRs, butcurrentGitUserNameand loading flags can carry stale values whenprimaryis unavailable (or while prior async calls are still settling). That can produce incorrectmine/author:mefiltering and stale loading UI.Suggested fix
const prepareCreateDialog = useCallback(() => { @@ setCreateBranches([]); setCreateBranchPullRequests([]); + setCreateGitUserName(""); + setCreateBranchesLoading(false); + setCreateBranchPullRequestsLoading(false); setLaneCreated(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx` around lines 1536 - 1572, Reset the create-dialog identity and loading state immediately before the primary-branch-dependent async work so stale values aren't shown when primary is absent or prior requests are pending: ensure you call setCreateGitUserName("") and setCreateBranchPullRequestsLoading(false) and setCreateBranchesLoading(false) (and optionally setCreateBranches([]) / setCreateBranchPullRequests([]) if you want empty lists) alongside the existing setCreateBranchPullRequests([]) / setLaneCreated(false) / createBaseBranchUserPickedRef.current = false at the top of prepareCreateDialog; keep the later window.ade.git.* and window.ade.prs.* calls only inside the primary check as they are now.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsx (1)
366-400: ⚡ Quick winAvoid wall-clock sleeps in this fallback test.
The 1.6s / 2.5s waits make the suite slower and still depend on scheduler timing. Switching this case to fake timers would make the retry assertions deterministic.
♻️ Suggested test refactor
- await new Promise((resolve) => setTimeout(resolve, 1_600)); + vi.advanceTimersByTime(1_600); + await Promise.resolve();Apply the same pattern to the later 2.5s idle check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsx` around lines 366 - 400, Replace the real-time sleeps with fake timers: in the test before triggering retries call vi.useFakeTimers(), render ChatIosSimulatorPanel (using installIosSimulatorApi to get api), and then advance timers via vi.advanceTimersByTime(...) inside act() to simulate the 1_600ms and 2_500ms delays instead of awaiting new Promise timeouts; after asserting api.startStream call counts and fallback behavior advance the fake timers as needed and finally call vi.useRealTimers() to restore; reference installIosSimulatorApi, api.startStream, emit (for stream-error), and ChatIosSimulatorPanel when locating where to replace the setTimeout-based waits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 1688-1690: The PR list-open command path currently returns a label
"PR list open" which prevents the PR table formatter from being applied; update
the branch that checks sub === "list-open" || sub === "open" || sub ===
"list-repo-open" to return the same label used by the prs list formatter (e.g.,
"prs-list") while keeping the steps unchanged (the actionCallStep("result",
"prs_list_open", {}) call should remain); this ensures the formatter inference
recognizes the command and preserves text-mode table output.
In `@apps/desktop/native/ios-sim-helpers/build.sh`:
- Around line 90-92: The current PRINT_JSON branch uses printf to emit JSON
which will break if any of the variables XCODE_VERSION, SOURCE_HASH, OUT_DIR,
CAPTURE, or INPUT contain quotes or backslashes; replace the printf call with a
real JSON encoder (e.g., use python -c, node -e, or jq -n) to construct and
print a properly escaped JSON object when PRINT_JSON == "1", ensuring the keys
"xcodeVersion","sourceHash","buildDir","capture","input" are populated from
those variables.
In `@apps/desktop/native/ios-sim-helpers/sim-capture.swift`:
- Around line 325-376: There is a data race on currentStream and
currentDeviceUdid because they are accessed from signal handlers (sigTerm,
sigInt) on the main queue and the background polling loop; introduce a dedicated
serial DispatchQueue (e.g., streamStateQueue) and wrap every read/write to
currentStream and currentDeviceUdid inside streamStateQueue.sync or async as
appropriate (apply to the signal handlers where you call currentStream?.stop()
and set currentStream/currentDeviceUdid, and to the polling loop locations that
read or mutate these variables and check/assign notifiedNoBoot), ensuring all
access paths (sigTerm, sigInt, and the DispatchQueue.global loop) use the same
queue to serialize access.
In `@apps/desktop/native/ios-sim-helpers/sim-input.m`:
- Around line 385-386: ensureHID() may fail but the code currently proceeds to
elog(@"[sim-input] ready"); instead of aborting; update the startup sequence in
sim-input.m to check the result of ensureHID() (or catch any NSError it
produces), and if initialization fails log a clear error via elog (including the
error details) and terminate the helper immediately (e.g., exit with non-zero or
return an error code) instead of printing "ready" and continuing; locate the
call to ensureHID() and replace the unconditional call+elog with a conditional
that fails fast on error.
In `@apps/desktop/src/main/services/ios/iosSimulatorService.ts`:
- Around line 3577-3581: The iosurfaceScreenMetrics map is not being cleared on
session shutdown/dispose so stale metrics persist and cause mis-normalized
coordinates; update the teardown/shutdown/dispose paths (where activeSession is
nulled, e.g. in shutdown(), dispose(), and the other teardown block around the
other session cleanup) to reset iosurfaceScreenMetrics (clear the map or assign
an empty object) for the UDID or entirely (iosurfaceScreenMetrics = {} or delete
the UDID key) so new sessions start with fresh metrics; ensure you apply the
same change in the other cleanup location mentioned in the review.
- Around line 4287-4310: The sticky fallback uses "anonymous" when there's no
activeSession but preferIndigoInput only compares against activeSession?.id, so
anonymous sessions never match and keep retrying Indigo; update
preferIndigoInput to compare iosurfaceInputStickyFallbackSessionId against
(activeSession?.id ?? "anonymous") (i.e., change the early-return check to use
activeSession?.id ?? "anonymous") so the "anonymous" sticky fallback prevents
Indigo retries for anonymous flows.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 5877-5878: The current truthy coercion of
keepSimulatorInBackground can mis-handle non-boolean payloads; change the
parsing so only a literal boolean is accepted and otherwise default to
true—locate the assignment to keepSimulatorInBackground in registerIpc.ts and
replace the Boolean((arg as {...})?.keepSimulatorInBackground ?? true) logic
with a type check that sets keepSimulatorInBackground = typeof (arg as {
keepSimulatorInBackground?: unknown } | null)?.keepSimulatorInBackground ===
'boolean' ? (arg as { keepSimulatorInBackground?: boolean
}).keepSimulatorInBackground : true; thereby ensuring only actual booleans are
respected and all other values (strings, numbers, null, undefined) fall back to
true.
In `@apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsx`:
- Around line 1244-1247: The preserveVisual path is currently clearing the MJPEG
`url` (in the `setLiveVisual` updater) which unmounts the canvas and causes
flicker; change the updater used where `preserveVisual` is checked so that for
`current?.kind === "mjpeg"` you only update `status` to "reconnecting" (and
optionally reset `error`) but do NOT set `url: null` — keep the existing `url`
while reconnecting, and only clear `url` in the hard-stop/error code paths where
the visual should truly be removed; apply the same change to the other
`setLiveVisual` sites that handle `"mjpeg"` (the occurrences referenced by the
reviewer).
- Around line 1757-1758: The watchdog early-return currently skips stall
detection for iosurface-indigo once a first frame has arrived because the
condition is `streamStatus.backend === "iosurface-indigo" && lastFrameMs`;
change the logic so we only skip the watchdog before the first frame on
iosurface-indigo (i.e., return only when backend is "iosurface-indigo" &&
!lastFrameMs) so that staleAfterMs and restart recovery still run after the
first frame; update the conditional near where `streamStatus`, `lastFrameMs`,
and `staleAfterMs` are computed in ChatIosSimulatorPanel to reflect this
corrected check.
In `@apps/desktop/src/renderer/components/lanes/BranchPickerView.tsx`:
- Around line 135-155: Replace the frozen "now" useMemo with a stateful ticking
timestamp: change const now = React.useMemo(() => Date.now(), []) to a useState
e.g. const [now, setNow] = React.useState(Date.now()), and add a React.useEffect
that sets a lightweight setInterval (e.g. 30s–60s) to call setNow(Date.now())
and clears the interval on cleanup so the component updates while the picker is
open; leave inputRef focus logic intact and ensure the existing useMemo for
filtered (which references now, prByBranch, parsed, parseSearchQuery,
findPrForBranch, matchesQuery) continues to depend on now so relative ages and
stale: filtering refresh.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx`:
- Around line 1536-1572: Reset the create-dialog identity and loading state
immediately before the primary-branch-dependent async work so stale values
aren't shown when primary is absent or prior requests are pending: ensure you
call setCreateGitUserName("") and setCreateBranchPullRequestsLoading(false) and
setCreateBranchesLoading(false) (and optionally setCreateBranches([]) /
setCreateBranchPullRequests([]) if you want empty lists) alongside the existing
setCreateBranchPullRequests([]) / setLaneCreated(false) /
createBaseBranchUserPickedRef.current = false at the top of prepareCreateDialog;
keep the later window.ade.git.* and window.ade.prs.* calls only inside the
primary check as they are now.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsx`:
- Around line 366-400: Replace the real-time sleeps with fake timers: in the
test before triggering retries call vi.useFakeTimers(), render
ChatIosSimulatorPanel (using installIosSimulatorApi to get api), and then
advance timers via vi.advanceTimersByTime(...) inside act() to simulate the
1_600ms and 2_500ms delays instead of awaiting new Promise timeouts; after
asserting api.startStream call counts and fallback behavior advance the fake
timers as needed and finally call vi.useRealTimers() to restore; reference
installIosSimulatorApi, api.startStream, emit (for stream-error), and
ChatIosSimulatorPanel when locating where to replace the setTimeout-based waits.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8cf11857-e12e-4411-b610-153c34b77ed1
⛔ Files ignored due to path filters (4)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/ios-simulator/README.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**
📒 Files selected for processing (31)
apps/ade-cli/README.mdapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/native/ios-sim-helpers/.gitignoreapps/desktop/native/ios-sim-helpers/README.mdapps/desktop/native/ios-sim-helpers/build.shapps/desktop/native/ios-sim-helpers/sim-capture.swiftapps/desktop/native/ios-sim-helpers/sim-input.mapps/desktop/src/main/services/git/gitOperationsService.branchSwitch.test.tsapps/desktop/src/main/services/git/gitOperationsService.tsapps/desktop/src/main/services/ios/iosSimulatorService.test.tsapps/desktop/src/main/services/ios/iosSimulatorService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsxapps/desktop/src/renderer/components/lanes/BranchPickerView.test.tsxapps/desktop/src/renderer/components/lanes/BranchPickerView.tsxapps/desktop/src/renderer/components/lanes/CreateLaneDialog.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/branchPickerSearch.test.tsapps/desktop/src/renderer/components/lanes/branchPickerSearch.tsapps/desktop/src/renderer/components/lanes/laneUtils.tsapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/git.tsapps/desktop/src/shared/types/iosSimulator.ts
…omments CI: convert idb_companion spawn errors into structured stream-error events and add disposed-checks to ensureCompanion's prewarm path so a fire-and-forget spawn does not bubble as an unhandled error after dispose. Review (CodeRabbit + Greptile): - ade-cli prs list-open table formatter label fix - native helpers: jq/python JSON, serial DispatchQueue for sim-capture state, ensureHID failure now exits non-zero - iosSimulatorService: clear iosurfaceScreenMetrics on shutdown/dispose; preferIndigoInput compares against fallback marker - registerIpc: explicit boolean handling for keepSimulatorInBackground - ChatIosSimulatorPanel: stable MJPEG url through preserveVisual; stall detection extended to iosurface-indigo with per-transport threshold - BranchPickerView: ticking now-state for relative ages - LanesPage: reset identity/loading state in prepareCreateDialog - prService: maxPages cap on listOpenPullRequests - branchPickerSearch: pr:open includes drafts Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
iOS simulator native helpers + sim-capture/sim-input bridges, ChatIosSimulatorPanel updates, lanes BranchPickerView, branch-picker search utilities, ADE CLI guidance + RPC tweaks, and feature docs sync. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…omments CI: convert idb_companion spawn errors into structured stream-error events and add disposed-checks to ensureCompanion's prewarm path so a fire-and-forget spawn does not bubble as an unhandled error after dispose. Review (CodeRabbit + Greptile): - ade-cli prs list-open table formatter label fix - native helpers: jq/python JSON, serial DispatchQueue for sim-capture state, ensureHID failure now exits non-zero - iosSimulatorService: clear iosurfaceScreenMetrics on shutdown/dispose; preferIndigoInput compares against fallback marker - registerIpc: explicit boolean handling for keepSimulatorInBackground - ChatIosSimulatorPanel: stable MJPEG url through preserveVisual; stall detection extended to iosurface-indigo with per-transport threshold - BranchPickerView: ticking now-state for relative ages - LanesPage: reset identity/loading state in prepareCreateDialog - prService: maxPages cap on listOpenPullRequests - branchPickerSearch: pr:open includes drafts Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
97af9ef to
5019589
Compare
Restores the early-return for iosurface-indigo after first frame so the test "does not restart an idle native stream after it has produced a frame" continues to pass. The CodeRabbit nit was minor and conflicts with this load-bearing test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
|
@greptile review |
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
Release Notes
New Features
ade git user-identityCLI command to retrieve git user configuration.ade prs list-openCLI command to list open pull requests.Improvements
Greptile Summary
This PR introduces the
iosurface-indigobackend for iOS Simulator streaming (native IOSurface frames + Indigo HID input), adds a rich branch picker with PR pills and commit metadata, and wires upade git user-identity/ade prs list-openCLI commands.startStreamdefault from"simctl-screenshot-poll"to"auto"now callsawait detectIosurfaceIndigoCapability(), which blocks for up to 120 s while the helper builds on first run. Any stream request that races the background warmup will stall that long before streaming begins.hashHelperSources(helperRoot)is called but its return value is discarded; the hash comparison againsthelpers.sourceHashis never performed.Confidence Score: 4/5
Safe to merge with the stream-startup blocking issue addressed; all other paths look correct.
One P1 finding (stream startup can block for up to 120 s on first call with the new
autodefault) caps the score at 4. The rest of the changes are well-structured with proper error handling and fallback chains.apps/desktop/src/main/services/ios/iosSimulatorService.ts — specifically the
startStreamfunction's use ofdetectIosurfaceIndigoCapabilityforautoresolution.Important Files Changed
iosurface-indigobackend with capability detection, helper build/cache, fallback chain, and Indigo input helper. One P1 issue: the newautodefault forstartStreamcan block for up to 120 s if the helper build hasn't finished warming up.listOpenPullRequests()andmaxPagesparam tofetchAllPages; previous unbounded-page concern is now addressed withmaxPages: 5.listBranchesto include per-branch commit metadata (SHA, date, author, message) and addsgetUserIdentity. Tab-delimited format with subject last avoids parsing issues.nowtimestamp usesuseState + setInterval(60s refresh), addressing the stale-time concern from a previous review.pr:opentoken now correctly includes drafts with an explanatory comment, addressing the previous review concern.<select>for branch import with the newBranchPickerViewsheet. PR pill and commit metadata are displayed on the selected-branch summary row.IosSimulatorStreamStatuswith latency percentiles, helper PID, input backend, fallback/degradation reasons, and theiosurface-indigobackend variant.BranchPullRequest,GitUserIdentity, and commit metadata fields toGitBranchSummary. Clean type additions with JSDoc.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[startStream called\ndefault: auto] --> B{backend === auto?} B -->|yes| C[detectIosurfaceIndigoCapability\nblocks if build in progress ⚠] B -->|no| D[Use requested backend directly] C --> E{iosurface available\n& <2 failures/60s?} E -->|yes| F[startIosurfaceIndigoStream] E -->|no| G{windowCaptureAllowed?} G -->|yes| H[startWindowCaptureStream] G -->|no| I{idb + idb_companion?} I -->|yes| J[startIdbMjpegStream] I -->|no| K[startSimctlPreview] F --> L{First frame within 5s?} L -->|yes| M[Stream running ✓] L -->|no| N[startFallbackAfterIosurfaceFailure\nrememberIosurfaceFailure ++] N --> G J --> O{idb-mjpeg exits?} O -->|yes, ffmpeg avail| P[startIdbH264FfmpegStream] O -->|yes, no ffmpeg| KComments Outside Diff (5)
apps/desktop/src/main/services/ios/iosSimulatorService.ts, line 1213-1216 (link)stdin.write()failure is silently ignored on backpressureWhen
sendIosurfaceInputCommandwrites to the input helper'sstdin, the code registers an empty"drain"listener whenwrite()returnsfalse, but theJSON.stringify({ id, ...command })payload is already flushed — it is NOT buffered and retried. If the pipe is currently under backpressure (write()→false), the payload is still written but the pending-map entry will time out after 5 s rather than getting an ack, and the caller will receive a spurious timeout error.Prompt To Fix With AI
apps/desktop/src/main/services/ios/iosSimulatorService.ts, line 184-188 (link)iosurfaceCapabilityCacheis a module-level variable and is never cleared on service disposal. If the user switches the selected Xcode (viaxcode-select -s) mid-session the stale cache will continue returning the old capability until restart. ThedetectIosurfaceIndigoCapabilityfunction correctly invalidates bydeveloperDir, but the settled fallback inpeekIosurfaceIndigoCapabilityForStatuswill still serve the cached value. AclearCache()export or a cleanup hook called fromdispose()would prevent this from silently causing the wrong backend to be chosen.Prompt To Fix With AI
apps/desktop/src/main/services/ios/iosSimulatorService.ts, line 919-940 (link)stopStream()has changedstreamStatus.backendInside
startIosurfaceIndigoStream, the startup timer checksstreamStatus.backend !== "iosurface-indigo"before acting, but the timer captureschildin a closure. AfterstopStream()is called and a new backend is started,streamProcess !== childguard correctly skips the callback — so this is fine when the stream is fully restarted. However, ifsetStreamStoppedis called in the meantime (e.g., viastopStream()),streamStatus.runningisfalse, so the early-return guard fires and bothstartFallbackAfterIosurfaceFailureAND the prior backend-stop code leavestreamProcess === null. The fallback then spawns a new process and emits another"stream-started"event when the UI may already be showing a stopped state from a user-initiated stop. Consider adding an explicitdisposed/stoppedflag to the timeout callback.Prompt To Fix With AI
apps/desktop/src/main/services/ipc/registerIpc.ts, line 5874-5454 (link)keepSimulatorInBackgroundflag is double-read fromargbutlaunch()already stores itIn the
iosSimulatorLaunchIPC handler,keepSimulatorInBackgroundis read fromarga second time here to decide whether to callprepareSimulatorWindowForCapture. Butlaunch()already processeslaunchArgs.keepSimulatorInBackground(defaulting totrue) and stores it inactiveSession.keepSimulatorInBackground. Reading it again from the rawargobject (with a different default logic) means the IPC layer and the service's internal flag could differ if the argument is absent. It is safer to rely on the returnedresult.session?.keepSimulatorInBackgroundfromlaunch().Prompt To Fix With AI
apps/desktop/src/main/services/ios/iosSimulatorService.ts, line 3443-3451 (link)startStreamcan block for up to 120 s on first call with newautodefaultThe default stream backend changed from
"simctl-screenshot-poll"to"auto". Withauto,startStreamnow callsawait detectIosurfaceIndigoCapability(), which may block for up to 120 seconds (buildIosurfaceIndigoHelperstimeout) if the stream is requested before the background warmup resolves. AnystartStream("auto")call that races the build shares the same pending Promise and stalls. Previously, a caller with nobackendarg would getsimctl-screenshot-pollimmediately.Consider using
peekIosurfaceIndigoCapabilityForStatusfor theautoresolution instartStream(its settled value is available instantly, falling back to the next backend while the build completes), or add a short max-wait timeout before degrading gracefully.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "ship: revert iosurface-indigo stall watc..." | Re-trigger Greptile