IOS Sim Refresh#239
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR spans desktop and iOS platforms with five independent feature areas: Codex plan-mode system prompts and plan-text tracking; iOS Indigo touch input with screen-dimension payloads and Xcode version detection; sync service platform-aware heartbeat limits and chat delivery awareness; iOS lane rebase UI refactoring to remove defer actions; and iOS work-chat delivery-state tracking for queued vs. sent messages. ChangesCodex Planning & Plan-Mode System Prompt
iOS Indigo Touch Input & Payload Construction
Sync Service: Platform-Aware Heartbeat & Chat Delivery Awareness
iOS Lanes Rebase UI Refactoring
iOS Work Chat: Message Delivery State & Queueing Logic
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)
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. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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 |
Owner
Author
|
@copilot review but do not make fixes |
Contributor
Here's the review — no changes made.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/ios/ADE/Services/SyncService.swift (1)
5512-5521:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't continue as healthy after
applyChangesfails.This catch path now NACKs the batch and sets
lastError, but it no longer degradesconnectionState/domain status or forces a recovery pass. Ifdatabase.applyChanges(...)throws, the app can stay connected while serving stale local data. Please mark sync as failed and trigger reconnect/full hydration here instead of resolving this as a normal success path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Services/SyncService.swift` around lines 5512 - 5521, When database.applyChanges(...) throws, do not resolve the request as a normal success; instead keep the NACK and set lastError, mark the sync/connection state as failed (e.g. set connectionState = .failed or call the existing method that marks sync failure), and trigger the recovery/reconnect/full-hydration flow before resolving. Replace the resolve(requestId: requestId, result: .success(payload)) call in this catch block with logic that sets the failed state and invokes the app's recovery/reconnect routine (the same code path used elsewhere for full rehydration/reconnect), then resolve or requeue the request only after recovery is scheduled.apps/ios/ADE/Views/Work/WorkSessionDestinationView.swift (1)
405-467:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate the auto-sent opening prompt with the same send eligibility as the composer.
This path still stages a
"sending"local echo and callssendChatMessage(...)even whencanSendChatMessagesis false. In other words, the UI can correctly disable Send for an ended or non-queueable offline session, but the automatic opening prompt still tries to send anyway.Proposed fix
`@MainActor` func sendInitialOpeningPromptIfNeeded() async { let prompt = trimmedInitialOpeningPrompt guard !prompt.isEmpty else { return } guard !sending else { return } + guard canSendChatMessages else { return } let promptKey = "\(sessionId)|\(prompt)" guard handledOpeningPromptKey != promptKey else { return } if transcript.contains(where: { envelope in if case .userMessage(let text, _, _, _, _) = envelope.event { return text.trimmingCharacters(in: .whitespacesAndNewlines) == prompt @@ `@MainActor` func stageInitialOpeningPromptEchoIfNeeded() { let prompt = trimmedInitialOpeningPrompt guard !prompt.isEmpty else { return } + guard canSendChatMessages else { return } let promptKey = "\(sessionId)|\(prompt)" guard stagedOpeningPromptKey != promptKey else { return } stagedOpeningPromptKey = promptKey localEchoMessages.append(WorkLocalEchoMessage( text: prompt,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Work/WorkSessionDestinationView.swift` around lines 405 - 467, The auto-send logic needs to respect the same send eligibility as the composer: in sendInitialOpeningPromptIfNeeded and stageInitialOpeningPromptEchoIfNeeded, gate staging and sending by the existing canSendChatMessages (or the same predicate used to enable the Send button) before creating a local echo or calling syncService.sendChatMessage(sessionId:text:); if canSendChatMessages is false, do not append a "sending" local echo or call sendChatMessage (you may still set handledOpeningPromptKey to avoid retry loops), and use sendWillQueueChatMessage only when queueing is allowed by that same predicate.apps/desktop/src/main/services/ios/iosSimulatorService.ts (1)
4469-4470:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject negative swipe durations here.
durationMsis only checked for finiteness, so negative values reach the Indigo path and behave differently from the idb fallback, which clamps them to0.01s. Tighten this todurationMs >= 0so both backends stay equivalent.Suggested fix
- if (durationMs != null && !Number.isFinite(durationMs)) throw new Error("durationMs must be a number."); + if (durationMs != null && (!Number.isFinite(durationMs) || durationMs < 0)) { + throw new Error("durationMs must be a non-negative number."); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ios/iosSimulatorService.ts` around lines 4469 - 4470, The current validation only ensures durationMs is finite which allows negative durations to pass through and diverge between Indigo and idb backends; update the check that validates durationMs (the line referencing durationMs and deltaValue) to reject negative values by requiring durationMs to be a finite number >= 0 (e.g., change the condition to !Number.isFinite(durationMs) || durationMs < 0) and update the thrown error message to indicate a non-negative duration, keeping the existing deltaValue validation as-is.apps/desktop/native/ios-sim-helpers/sim-input.m (1)
418-462:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate touch/swipe numbers before coercing them.
This path now ACKs malformed payloads as success: missing
x/ybecome0, and missingwidth/heightbecome1, so bad point-space commands can turn into edge taps instead of a typed failure. JSONnullvalues can also tripdoubleValueunexpectedly here. Please reject non-finite required fields up front, and only treat omittedwidth/heightas legacy normalized input whenx/yare already in[0,1].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/native/ios-sim-helpers/sim-input.m` around lines 418 - 462, The touch/tap/swipe handlers accept nil/null or non-finite numbers and silently coerce them (e.g. x/y -> 0, width/height -> 1); update validation so sendTouch is only called with finite required coordinates and safe sizes: for all handlers (the touch branch, tap branch using x/y/hold, and swipe branch using startX/startY/endX/endY/durationMs) check that each required numeric field is present, non-null and isfinite() (or equivalent) before coercion; if validation fails return NO and populate errorOut; for width/height only allow the legacy default (positiveOrDefault) when width/height are omitted AND the provided x/y (or startX/startY/endX/endY) are already in [0,1]; otherwise treat missing/invalid width/height as an error. Ensure JSON nulls are detected (not just doubleValue) so malformed payloads are rejected rather than turned into edge taps; reference sendTouch, positiveOrDefault, MouseEvent* and MouseDirection* to locate where to add these checks.
🧹 Nitpick comments (5)
apps/ios/ADE/Views/Work/WorkModels.swift (1)
40-45: ⚡ Quick winUse a typed delivery-state enum instead of raw strings.
This PR introduces a real state machine for local echoes, but
deliveryStateis still a free-formString?. A typo in any of the touched views/helpers will silently break badges/timeline rendering. I'd strongly prefer a shared enum here and inWorkChatMessageso the compiler enforces the allowed states.♻️ Suggested direction
+enum WorkMessageDeliveryState: String, Equatable { + case sending + case queued +} + struct WorkLocalEchoMessage: Identifiable, Equatable { let id = UUID().uuidString let text: String let timestamp: String - var deliveryState: String? = nil + var deliveryState: WorkMessageDeliveryState? = nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Work/WorkModels.swift` around lines 40 - 45, Replace the free-form deliveryState String? on WorkLocalEchoMessage with a strongly-typed enum (e.g., DeliveryState) and update any related model WorkChatMessage to use the same enum so the compiler enforces valid states; define DeliveryState as a Codable/Equatable RawRepresentable (String) enum listing the allowed cases (pending, sent, failed, etc.), change WorkLocalEchoMessage.deliveryState to DeliveryState? and update all usages/comparisons, initializers and JSON (de)coding paths to convert to/from the enum instead of raw strings.apps/ios/ADE/Views/Work/WorkChatSessionView.swift (1)
155-186: ⚡ Quick winCache active-turn detection instead of rescanning the transcript each render.
transcriptIndicatesActiveTurnis a computed property, so every unrelatedWorkChatSessionViewstate update re-walks the entire transcript. This view already caches timeline-derived state to avoid that pattern; moving this flag into the existing rebuild path would keep long chats from paying an extra O(n) scan on every render.As per coding guidelines,
apps/ios/**/*.swift: iOS Swift app — check for memory management, Swift conventions, and proper SwiftUI patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Work/WorkChatSessionView.swift` around lines 155 - 186, transcriptIndicatesActiveTurn is expensive because it's a computed property that rescans transcript on every render; convert it to a cached stored Bool that's updated during the existing timeline rebuild path (instead of being recomputed in the view). Add a stored property (e.g. a private var or `@State/`@Binding as appropriate) like transcriptIndicatesActiveTurnCached, compute its value once inside the timeline/session rebuild function (the routine that currently derives timeline state) by reusing the same logic that inspects transcript and envelope.event/.done, and then have the WorkChatSessionView read that cached property rather than the computed transcriptIndicatesActiveTurn; ensure the cached value is updated whenever transcript changes or the rebuild runs.apps/desktop/src/main/services/ios/iosSimulatorService.test.ts (1)
503-513: ⚡ Quick winCover the validation branches too.
This only locks in the happy path. Please add one failure case for non-finite coordinates and one for invalid screen metrics, since those branches are what protect the Indigo fallback path from regressions.
Suggested additions
it("builds point-space Indigo input payloads with screen dimensions", () => { expect(iosurfaceInputPointPayload( { x: 201, y: 830 }, { width: 402, height: 874 }, )).toEqual({ x: 201, y: 830, width: 402, height: 874, }); }); + + it("rejects non-finite point coordinates", () => { + expect(() => iosurfaceInputPointPayload( + { x: Number.NaN, y: 830 }, + { width: 402, height: 874 }, + )).toThrow(/point coordinates are required/i); + }); + + it("rejects invalid screen metrics", () => { + expect(() => iosurfaceInputPointPayload( + { x: 201, y: 830 }, + { width: 0, height: 874 }, + )).toThrow(/screen metrics are required/i); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ios/iosSimulatorService.test.ts` around lines 503 - 513, Add two unit tests for iosurfaceInputPointPayload to cover validation branches: one that calls iosurfaceInputPointPayload with non-finite coordinates (e.g., x: Infinity or y: NaN) and asserts the function fails the validation (expect a thrown error or a falsy/invalid result per the function's contract); and one that calls iosurfaceInputPointPayload with invalid screen metrics (e.g., width/height <= 0 or NaN) and similarly asserts the validation branch is taken (throw or falsy result). Ensure both tests reference iosurfaceInputPointPayload so the non-happy-path branches are exercised.apps/desktop/src/main/services/ai/tools/systemPrompt.test.ts (1)
46-56: ⚡ Quick winAdd a non-interactive Codex plan-mode regression case.
Line 46 currently validates only positive Codex-plan wording. Please add a case for
interactive: falseto assert the prompt does not nudgerequest_user_inputand does not include the non-CodexexitPlanModeapproval path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/tools/systemPrompt.test.ts` around lines 46 - 56, Add a regression test that calls buildCodingAgentSystemPrompt with permissionMode: "plan", runtime: "codex-cli", and interactive: false, and assert the returned prompt does NOT contain the strings "request_user_input" and "exitPlanMode"; this complements the existing positive case by verifying non-interactive Codex plan prompts neither nudge for user input nor include the non-Codex exitPlanMode approval path.apps/desktop/src/main/services/chat/agentChatService.ts (1)
2907-2919: ⚡ Quick winType this helper against the Codex input contract.
Returning
Record<string, unknown>drops compile-time validation for the payload we send to Codex, so field drift intype/text/text_elementsbecomes a runtime-only failure in the new ADE-context path. Please return the concrete Codex input-item type here and run the desktop typecheck/build/lint pass after tightening it. As per coding guidelines,apps/desktop/**/*.{ts,tsx}: Run typecheck withnpm --prefix apps/desktop run typecheck, run build withnpm --prefix apps/desktop run build, and run lint withnpm --prefix apps/desktop run lintfor desktop app validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 2907 - 2919, The function buildCodexAdeContextInput currently returns Record<string, unknown>; change its return signature to the concrete Codex input-item type used by our Codex/ADE pipeline (replace Record<string, unknown> with the repository's Codex input item interface—e.g., CodexInputItem or the exact name exported by the Codex types), ensure the object shape matches that interface (type: "text", text: string, text_elements: the correct array element type), add any necessary imports, then run the desktop validations: npm --prefix apps/desktop run typecheck, npm --prefix apps/desktop run build, and npm --prefix apps/desktop run lint to confirm the fix.
🤖 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/desktop/src/main/services/ai/tools/systemPrompt.ts`:
- Around line 149-153: The Codex-specific guidance currently unconditionally
tells the agent to use request_user_input when runtime === "codex-cli"; change
the prompt generation so the sentence that says "use request_user_input for
important clarifications when needed" is only included when the session is
interactive (i.e., interactive !== false), and when interactive === false
replace or augment that guidance with an explicit instruction to continue
without asking questions and proceed with the best-effort plan; update the
branch that builds the array for runtime === "codex-cli" (the two prompt strings
shown) to conditionally include the request_user_input text based on the
interactivity flag.
In `@apps/desktop/src/main/services/chat/agentChatService.test.ts`:
- Around line 4306-4374: Add Node typings and remove the deprecated compiler
option in the desktop tsconfig: install `@types/node` as a devDependency, add
"types": ["node"] under compilerOptions in the tsconfig (so the missing 'node'
definition TS2688 is resolved), and remove the deprecated "baseUrl" option from
the tsconfig.json (to avoid the TS5101 deprecation/error in future TS releases);
after making these edits run the validation commands: npm --prefix apps/desktop
run test, npm --prefix apps/desktop run typecheck, npm --prefix apps/desktop run
build, and npm --prefix apps/desktop run lint.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 10217-10225: The loop emitting plan approvals currently runs
unconditionally and can emit approvals even when a turn later fails; modify the
logic around runtime.planTextByItemId so you only call
emitCodexPlanTextApproval(managed, runtime, planText,
runtime.itemTurnIdByItemId.get(planItemId) ?? turnId) when the turn completed
successfully (check the turn completion/status branch) and otherwise skip
emitting and simply clear runtime.planTextByItemId; apply the same gating change
to the other similar block referenced (the one around lines 10231-10240) so
failures/cancels do not produce plan_approval events.
- Around line 10575-10588: Currently a new randomUUID is created for every plan
delta when params.itemId is missing, preventing fragments from grouping; change
the fallback key to a stable per-turn key instead of randomUUID: derive itemId
from params.itemId || turnIdFromParams || runtime.itemTurnIdByItemId.get(turnId)
|| runtime.activeTurnId (e.g. prefix with "__turn-{turnId}" so it's a distinct
stable key), use that stable key when reading/updating runtime.planTextByItemId
and when calling evictOldestEntries/emitChatEvent, and ensure when a real itemId
later arrives you migrate/merge buffered text from the per-turn key into the
real itemId and update runtime.itemTurnIdByItemId accordingly rather than using
randomUUID().
In `@apps/desktop/src/main/services/sync/syncHostService.test.ts`:
- Around line 293-300: The test asserting a wider heartbeat grace window for
mobile peers is a pure helper test and should not be skipped with the CRSQLite
gate; move the it("allows a wider heartbeat grace window for mobile peers", ...)
block out of the surrounding describe.skipIf(!isCrsqliteAvailable()) so it
always runs, keeping the assertion calls to
syncHeartbeatMissLimitForPeerMetadata({ platform: "iOS", deviceType: "phone" })
and syncHeartbeatMissLimitForPeerMetadata({ platform: "unknown", deviceType:
"phone" }) intact and unchanged; ensure the moved test remains in the same test
file alongside other non-CRSQLite-dependent tests.
In `@apps/ios/ADE/Views/Lanes/LaneComponents.swift`:
- Around line 534-536: The LaneCardRebaseWarning view is rendered visually but
not exposed to VoiceOver because LaneStackCard uses an explicit
accessibilityLabel (stackCardAccessibilityLabel) that omits the warning; update
the stackCardAccessibilityLabel computation (or the accessibilityLabel applied
to LaneStackCard) to append the rebase warning text when rebaseWarning is
non-nil (e.g., include rebaseWarning.summary or presentation.summary from the
rebaseWarning/presentation passed to LaneCardRebaseWarning) so the
accessibilityLabel includes both the existing stackCardAccessibilityLabel
content and the inline warning summary; alternatively, if you prefer
component-level accessibility, set an accessibilityLabel on
LaneCardRebaseWarning that returns the warning summary and ensure
LaneStackCard’s overall label concatenates it.
- Around line 556-562: The computed property rebaseWarning currently returns a
rebaseSuggestion before checking autoRebaseStatus, which can hide
higher-severity auto-rebase errors; change the order so
snapshot.autoRebaseStatus is evaluated first (and if status.state !=
"autoRebased" return .autoRebase(state:status.state,message:status.message)),
then fall back to snapshot.rebaseSuggestion (ensuring dismissedAt == nil) to
return .suggestion(behindCount:hasPr:), referencing the rebaseWarning var,
snapshot.autoRebaseStatus, snapshot.rebaseSuggestion and
LaneCardRebaseWarningPresentation.
In `@apps/ios/ADE/Views/Lanes/LaneDetailRebaseBanner.swift`:
- Line 72: The accessibilityLabel currently only uses bodyCopy and omits visible
dynamic badges (hasPr and behindCount); update the accessibilityLabel on
LaneDetailRebaseBanner to include those state values by composing a single
descriptive string (e.g., combine bodyCopy with hasPr presence and behindCount
number) so VoiceOver announces the PR status and how many commits behind; locate
the .accessibilityLabel("Rebase suggested. \(bodyCopy)") modifier and replace it
with a composed label that references bodyCopy, hasPr, and behindCount (ensuring
any optional values are safely formatted).
In `@apps/ios/ADE/Views/Work/WorkSessionDestinationView`+Actions.swift:
- Around line 12-30: sendMessage(_:) currently only filters duplicates/empty
input but still appends a WorkLocalEchoMessage and calls
syncService.sendChatMessage even when the live session is no longer sendable;
add the same live-session send-eligibility guard the composer uses at the start
of sendMessage(_:), and if the session is not sendable return early so you do
not append to localEchoMessages, set sending, or call
syncService.sendChatMessage(sessionId:text:). Ensure you reference the same
condition/flag used by the composer (the live-session send gate) so
updateLocalEchoDeliveryState, reconcileLocalEchoMessages, and other downstream
calls only run when the send gate allows sending.
---
Outside diff comments:
In `@apps/desktop/native/ios-sim-helpers/sim-input.m`:
- Around line 418-462: The touch/tap/swipe handlers accept nil/null or
non-finite numbers and silently coerce them (e.g. x/y -> 0, width/height -> 1);
update validation so sendTouch is only called with finite required coordinates
and safe sizes: for all handlers (the touch branch, tap branch using x/y/hold,
and swipe branch using startX/startY/endX/endY/durationMs) check that each
required numeric field is present, non-null and isfinite() (or equivalent)
before coercion; if validation fails return NO and populate errorOut; for
width/height only allow the legacy default (positiveOrDefault) when width/height
are omitted AND the provided x/y (or startX/startY/endX/endY) are already in
[0,1]; otherwise treat missing/invalid width/height as an error. Ensure JSON
nulls are detected (not just doubleValue) so malformed payloads are rejected
rather than turned into edge taps; reference sendTouch, positiveOrDefault,
MouseEvent* and MouseDirection* to locate where to add these checks.
In `@apps/desktop/src/main/services/ios/iosSimulatorService.ts`:
- Around line 4469-4470: The current validation only ensures durationMs is
finite which allows negative durations to pass through and diverge between
Indigo and idb backends; update the check that validates durationMs (the line
referencing durationMs and deltaValue) to reject negative values by requiring
durationMs to be a finite number >= 0 (e.g., change the condition to
!Number.isFinite(durationMs) || durationMs < 0) and update the thrown error
message to indicate a non-negative duration, keeping the existing deltaValue
validation as-is.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 5512-5521: When database.applyChanges(...) throws, do not resolve
the request as a normal success; instead keep the NACK and set lastError, mark
the sync/connection state as failed (e.g. set connectionState = .failed or call
the existing method that marks sync failure), and trigger the
recovery/reconnect/full-hydration flow before resolving. Replace the
resolve(requestId: requestId, result: .success(payload)) call in this catch
block with logic that sets the failed state and invokes the app's
recovery/reconnect routine (the same code path used elsewhere for full
rehydration/reconnect), then resolve or requeue the request only after recovery
is scheduled.
In `@apps/ios/ADE/Views/Work/WorkSessionDestinationView.swift`:
- Around line 405-467: The auto-send logic needs to respect the same send
eligibility as the composer: in sendInitialOpeningPromptIfNeeded and
stageInitialOpeningPromptEchoIfNeeded, gate staging and sending by the existing
canSendChatMessages (or the same predicate used to enable the Send button)
before creating a local echo or calling
syncService.sendChatMessage(sessionId:text:); if canSendChatMessages is false,
do not append a "sending" local echo or call sendChatMessage (you may still set
handledOpeningPromptKey to avoid retry loops), and use sendWillQueueChatMessage
only when queueing is allowed by that same predicate.
---
Nitpick comments:
In `@apps/desktop/src/main/services/ai/tools/systemPrompt.test.ts`:
- Around line 46-56: Add a regression test that calls
buildCodingAgentSystemPrompt with permissionMode: "plan", runtime: "codex-cli",
and interactive: false, and assert the returned prompt does NOT contain the
strings "request_user_input" and "exitPlanMode"; this complements the existing
positive case by verifying non-interactive Codex plan prompts neither nudge for
user input nor include the non-Codex exitPlanMode approval path.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 2907-2919: The function buildCodexAdeContextInput currently
returns Record<string, unknown>; change its return signature to the concrete
Codex input-item type used by our Codex/ADE pipeline (replace Record<string,
unknown> with the repository's Codex input item interface—e.g., CodexInputItem
or the exact name exported by the Codex types), ensure the object shape matches
that interface (type: "text", text: string, text_elements: the correct array
element type), add any necessary imports, then run the desktop validations: npm
--prefix apps/desktop run typecheck, npm --prefix apps/desktop run build, and
npm --prefix apps/desktop run lint to confirm the fix.
In `@apps/desktop/src/main/services/ios/iosSimulatorService.test.ts`:
- Around line 503-513: Add two unit tests for iosurfaceInputPointPayload to
cover validation branches: one that calls iosurfaceInputPointPayload with
non-finite coordinates (e.g., x: Infinity or y: NaN) and asserts the function
fails the validation (expect a thrown error or a falsy/invalid result per the
function's contract); and one that calls iosurfaceInputPointPayload with invalid
screen metrics (e.g., width/height <= 0 or NaN) and similarly asserts the
validation branch is taken (throw or falsy result). Ensure both tests reference
iosurfaceInputPointPayload so the non-happy-path branches are exercised.
In `@apps/ios/ADE/Views/Work/WorkChatSessionView.swift`:
- Around line 155-186: transcriptIndicatesActiveTurn is expensive because it's a
computed property that rescans transcript on every render; convert it to a
cached stored Bool that's updated during the existing timeline rebuild path
(instead of being recomputed in the view). Add a stored property (e.g. a private
var or `@State/`@Binding as appropriate) like transcriptIndicatesActiveTurnCached,
compute its value once inside the timeline/session rebuild function (the routine
that currently derives timeline state) by reusing the same logic that inspects
transcript and envelope.event/.done, and then have the WorkChatSessionView read
that cached property rather than the computed transcriptIndicatesActiveTurn;
ensure the cached value is updated whenever transcript changes or the rebuild
runs.
In `@apps/ios/ADE/Views/Work/WorkModels.swift`:
- Around line 40-45: Replace the free-form deliveryState String? on
WorkLocalEchoMessage with a strongly-typed enum (e.g., DeliveryState) and update
any related model WorkChatMessage to use the same enum so the compiler enforces
valid states; define DeliveryState as a Codable/Equatable RawRepresentable
(String) enum listing the allowed cases (pending, sent, failed, etc.), change
WorkLocalEchoMessage.deliveryState to DeliveryState? and update all
usages/comparisons, initializers and JSON (de)coding paths to convert to/from
the enum instead of raw strings.
🪄 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: bda03071-74fe-4822-b368-fc419656e684
⛔ Files ignored due to path filters (5)
docs/features/chat/agent-routing.mdis excluded by!docs/**docs/features/chat/transcript-and-turns.mdis excluded by!docs/**docs/features/ios-simulator/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/remote-commands.mdis excluded by!docs/**
📒 Files selected for processing (27)
apps/desktop/native/ios-sim-helpers/README.mdapps/desktop/native/ios-sim-helpers/sim-input.mapps/desktop/src/main/services/ai/tools/systemPrompt.test.tsapps/desktop/src/main/services/ai/tools/systemPrompt.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/ios/iosSimulatorService.test.tsapps/desktop/src/main/services/ios/iosSimulatorService.tsapps/desktop/src/main/services/sync/syncHostService.test.tsapps/desktop/src/main/services/sync/syncHostService.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.tsapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Lanes/LaneComponents.swiftapps/ios/ADE/Views/Lanes/LaneDetailGitSection.swiftapps/ios/ADE/Views/Lanes/LaneDetailRebaseBanner.swiftapps/ios/ADE/Views/Lanes/LaneDetailScreen.swiftapps/ios/ADE/Views/Lanes/LaneListViewParts.swiftapps/ios/ADE/Views/LanesTabView.swiftapps/ios/ADE/Views/Work/WorkChatSessionView.swiftapps/ios/ADE/Views/Work/WorkModels.swiftapps/ios/ADE/Views/Work/WorkNavigationAndTranscriptHelpers.swiftapps/ios/ADE/Views/Work/WorkPreviews.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView+Actions.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView.swiftapps/ios/ADE/Views/Work/WorkTimelineHelpers.swiftapps/ios/ADETests/ADETests.swift
💤 Files with no reviewable changes (3)
- apps/ios/ADE/Views/LanesTabView.swift
- apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift
- apps/ios/ADE/Views/Lanes/LaneListViewParts.swift
Owner
Author
|
@copilot review but do not make fixes |
Contributor
Updated review, incorporating Greptile's findings and a correction to my previous note — no changes made. Correction from previous review
This was wrong. In the actual implementation, Greptile P2 finding: dead exported Confirmed. Remaining findings from previous review (still valid) All of the following from my prior review stand:
|
This was referenced May 3, 2026
Merged
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
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.
Summary
Refresh the ADE iOS simulator control path and tighten mobile sync/chat behavior for the iOS companion.
What Changed
chat.senddelivery while reconnecting.Validation
npm --prefix apps/desktop run test -- src/main/services/ai/tools/systemPrompt.test.ts src/main/services/chat/agentChatService.test.ts src/main/services/ios/iosSimulatorService.test.ts src/main/services/sync/syncHostService.test.ts src/main/services/sync/syncRemoteCommandService.test.tsnpm --prefix apps/desktop run typechecknpm --prefix apps/desktop run lint(passes with existing warnings)npm --prefix apps/desktop run buildbash apps/desktop/native/ios-sim-helpers/build.sh --smoke --print-jsonxcodebuild test -project apps/ios/ADE.xcodeproj -scheme ADE -destination id=2CD8BD1C-C5F5-4B9D-B446-803488E4F559 -only-testing:ADETests/ADETests/testSyncAutomaticReconnectWaitsForLiveLanDiscoveryWithoutTailnet -only-testing:ADETests/ADETests/testSyncForegroundReconnectStartRequiresAutomaticRoute -only-testing:ADETests/ADETests/testSyncMessageTooLongTransportFailureForcesErrorState -only-testing:ADETests/ADETests/testSyncClientHeartbeatUsesHalfServerIntervalWithBounds -only-testing:ADETests/ADETests/testRemoteCommandPolicyQueuesChatSendWhenOffline -only-testing:ADETests/ADETests/testWorkChatQueuedSendRequiresLiveSession -only-testing:ADETests/ADETests/testWorkTimelineCarriesQueuedLocalEchoDeliveryState -only-testing:ADETests/ADETests/testSyncChangesetBatchPayloadDecodesLegacyBatchWithoutBatchIdnode scripts/validate-docs.mjsRisks
Summary by CodeRabbit
Release Notes
New Features
Improvements
Greptile Summary
This PR refreshes the iOS simulator control path (Xcode 26 dual mouse-event signatures, point-space touch payloads), adds a native Codex plan-approval workflow with streaming plan text, and improves mobile sync resilience (client heartbeats, mobile heartbeat grace, changeset retry, queued
chat.senddelivery, legacybatchIddecode). The changes are well-tested with dedicated unit tests covering the new sync, plan, and queue behaviours.Confidence Score: 4/5
Safe to merge; findings are edge-case P2s that do not break normal flows.
The PR is thoroughly tested and the architecture is sound. The highest-confidence concern is the plan-approval persistence after a failed turn (when the failure arrives after an item/completed plan event), which is untested and could surface a stale approval prompt — but approving it is recoverable. The connectionState: .connecting during teardown is a UX inaccuracy rather than a data issue. No P0/P1 bugs were found.
apps/desktop/src/main/services/chat/agentChatService.ts (plan approval lifecycle on failed turns), apps/ios/ADE/Services/SyncService.swift (connectionState during relay-loop failure)
Important Files Changed
Sequence Diagram
sequenceDiagram participant iOS as iOS App participant Desktop as Desktop Host participant Codex as Codex App-Server Note over iOS, Desktop: Mobile Sync — client heartbeat iOS->>Desktop: hello (connect) Desktop-->>iOS: hello_ack (heartbeatIntervalMs) loop every serverMs/2 (5-25 s) iOS->>Desktop: heartbeat {kind:ping, dbVersion} end Note over iOS, Desktop: Queued chat.send while reconnecting iOS->>Desktop: chat.send (queueable=true) Desktop-->>iOS: {queued:true} iOS->>iOS: echo deliveryState = queued Desktop->>Desktop: queue until live session Desktop->>Codex: turn/start Codex-->>Desktop: turn/started Note over Desktop, Codex: Codex plan-mode approval Desktop->>Codex: turn/start (collaborationMode=plan) Codex-->>Desktop: item/plan/delta (streaming) Desktop->>Desktop: accumulate planTextByItemId Codex-->>Desktop: item/completed {type:plan} Desktop->>Desktop: emitCodexPlanTextApproval Desktop-->>iOS: approval_request {kind:plan_approval} iOS->>Desktop: respondToInput (accept) Desktop->>Desktop: permissionMode to edit, interactionMode to default Desktop->>Codex: turn/start (collaborationMode=default)Comments Outside Diff (1)
apps/desktop/src/main/services/ios/iosSimulatorService.ts, line 329-342 (link)normalizeIosSimulatorPointForIndigonow has no production call sites — bothtapanddragwere migrated toiosurfaceInputPointPayloadin this PR. The function is still exported and tested, but shipping a dead public symbol can cause confusion about which API callers should use for Indigo input coordinates. If the function is kept only as a test utility, it should be unexported; if it's meant to be called externally in the future, a comment explaining that intent would help.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix: address PR review feedback" | Re-trigger Greptile