Mobile Connection Fixes#226
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThe PR renames many Codex-suffixed OpenAI model IDs to their canonical forms (e.g., Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
Owner
Author
|
@codex review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b690f6a7f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+178
to
+184
| if (pendingOutboundChangeset) { | ||
| if (nowMs - pendingOutboundChangeset.sentAtMs >= CHANGESET_ACK_TIMEOUT_MS) { | ||
| pendingOutboundChangeset.sentAtMs = nowMs; | ||
| pendingOutboundChangeset.retryCount += 1; | ||
| sendOutboundChangeset(pendingOutboundChangeset); | ||
| } | ||
| return; |
There was a problem hiding this comment.
This retry path resends any existing pendingOutboundChangeset as soon as the socket is open, but the new ack-based state is never scoped to a specific host/session. If the peer disconnects from host A with an unacked batch and then connects to host B, this branch will replay A’s pending batch to B after the timeout, which can leak/project-cross data and advance local sync cursor state against the wrong host. Please clear or rebind pending outbound batches when the connection target changes (or on disconnect) before allowing retries.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/main/services/chat/agentChatService.ts (1)
13744-13750:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate persisted reasoning effort before
thread/resume.These resume paths now send
managed.session.reasoningEffortstraight back to Codex, butreadPersistedState()only trims/lowercases that field. A stale or unknown value can survive restart and get replayed intothread/resume, which can break restore or keep the session stuck in a fallback/coercion loop. Mirror the validation you already do instartFreshCodexThread()before building both resume requests.💡 Suggested fix
+const resolvedResumeReasoningEffort = validateReasoningEffort( + "codex", + normalizeReasoningEffort( + managed.session.reasoningEffort ?? readPersistedState(sessionId)?.reasoningEffort, + ), +) ?? DEFAULT_REASONING_EFFORT; + const resumeResponse = await runtime.request<CodexThreadLifecycleResponse>("thread/resume", { threadId: threadIdToResume, model: managed.session.model, cwd: managed.laneWorktreePath, - reasoningEffort: managed.session.reasoningEffort ?? readPersistedState(sessionId)?.reasoningEffort ?? DEFAULT_REASONING_EFFORT, + reasoningEffort: resolvedResumeReasoningEffort, ...codexPolicyArgs(codexPolicy), persistExtendedHistory: true });Apply the same normalization in the
resumeSession()thread/resumecall as well.Also applies to: 14537-14544
🤖 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 13744 - 13750, The resume path is passing managed.session.reasoningEffort (and the fallback from readPersistedState()) directly into runtime.request("thread/resume"), which can replay stale/unknown values; mirror the validation/normalization performed in startFreshCodexThread() by normalizing/trimming/lowercasing and mapping reasoningEffort to allowed values (or falling back to DEFAULT_REASONING_EFFORT) before building the resume request. Locate the runtime.request("thread/resume") call where resumeResponse is assigned and replace the inline managed.session.reasoningEffort/readPersistedState(...) expression with the same normalized value-producing logic used by startFreshCodexThread(); do the same for the other resume occurrence that mirrors this pattern.apps/ios/ADE/Views/Work/WorkModelCatalog.swift (1)
313-314:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeduplicate live models by equivalence, not raw id.
The catalog lookup/sort path is now equivalence-aware, but
workDeduplicatedModelOptionsstill keys on the literalmodel.id. If the host returns bothgpt-5.4andopenai/gpt-5.4, both survive here and the picker shows duplicate rows for the same logical model.Suggested fix
private func workDeduplicatedModelOptions(_ models: [WorkModelOption]) -> [WorkModelOption] { - var seen = Set<String>() + var seen = Set<String>() var deduplicated: [WorkModelOption] = [] for model in models { - if seen.insert(model.id).inserted { + let keys = Set(workModelLookupKeys(model.id)) + if seen.isDisjoint(with: keys) { + seen.formUnion(keys) deduplicated.append(model) } } return deduplicated }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Work/WorkModelCatalog.swift` around lines 313 - 314, workDeduplicatedModelOptions currently deduplicates by the literal model.id which allows duplicates like "gpt-5.4" and "openai/gpt-5.4" to both appear; change the dedupe key to the model's equivalence identifier (e.g., model.equivalenceId / model.equivalenceKey or a normalized id derived from workModelEquivalence logic) so options are collapsed by logical equivalence rather than raw id, and when multiple implementations exist choose a deterministic winner (prefer curated/provider-preferred or first-seen) so workDeduplicatedModelOptions returns one entry per equivalence and the downstream sort call (workModelSortOrder) operates on equivalence-aware results.apps/ios/ADE/Services/SyncService.swift (1)
1335-1343:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftScope persisted mutation state to the paired host.
These new records are keyed only by project/site. If the user forgets one desktop and pairs another, queued commands/file writes and outbound cursor/pending-batch state survive and can either replay against the new host or suppress needed resends because the cursor looks already acked. Include a stable host identity in both the queued-operation payload and the outbound-sync storage key, or clear these stores when the host changes.
Also applies to: 3991-4001, 5754-5768
🤖 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 1335 - 1343, The PendingOperation struct and outbound-sync storage use only project/site keys and can mix state between different paired hosts; add a stable host identity (e.g., hostId: String) to the PendingOperation model and include that hostId when building the outbound-sync storage key, and update any enqueue/dequeue and read/write logic (the code paths that serialize/deserialize PendingOperation and construct outbound-sync keys) to persist and check hostId; alternatively, detect host change and clear related queued-operation and outbound-sync stores when the current paired host id differs from stored hostId to prevent cross-host replay/suppression.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsx (1)
1204-1227: 💤 Low valueConsider using
vi.useFakeTimers()instead of a hardcoded delay.The 550ms
setTimeoutdelay to assert thatreadTranscriptTailwas not called introduces potential test flakiness. If the implementation timing changes, this test could start failing or produce false positives. Using Vitest's fake timers would make this more deterministic.♻️ Proposed refactor using fake timers
it("does not hydrate hidden inactive chat tiles", async () => { + vi.useFakeTimers(); const session = buildSession("hidden-inactive-chat", { title: "Hidden inactive chat", }); installAdeMocks({ sessions: [session] }); const readTranscriptTail = vi.fn().mockResolvedValue(""); window.ade.sessions.readTranscriptTail = readTranscriptTail as any; render( <MemoryRouter> <AgentChatPane laneId={session.laneId} lockSessionId={session.sessionId} hideSessionTabs initialSessionSummary={session} layoutVariant="grid-tile" isTileActive={false} /> </MemoryRouter>, ); - await new Promise((resolve) => window.setTimeout(resolve, 550)); + await vi.advanceTimersByTimeAsync(1000); expect(readTranscriptTail).not.toHaveBeenCalled(); + vi.useRealTimers(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsx` around lines 1204 - 1227, Replace the hard 550ms setTimeout with Vitest fake timers to make the assertion deterministic: call vi.useFakeTimers() before rendering the AgentChatPane test, render the component, advance timers (e.g., vi.advanceTimersByTime or vi.runAllTimers) instead of awaiting setTimeout, then assert readTranscriptTail was not called and finally restore with vi.useRealTimers(); target the test case that references readTranscriptTail, AgentChatPane, hideSessionTabs and isTileActive to locate where to apply the change.apps/ios/ADE/Views/Work/WorkNewChatSheet.swift (1)
400-408: ⚡ Quick winCanonicalize preserved selections to the loaded model id.
If this branch keeps an equivalent selection,
selectedModelIdstays on the old alias instead of the fetched model’s.id. The screen still resolvesselectedModel, butsubmit()sends the stale identifier back tocreateChatSession.WorkSessionSettingsSheet+Actions.loadModels()already normalizes this case; this helper should do the same.♻️ Suggested adjustment
models = loadedModels - if resetSelection || loadedModels.contains(where: { workModelIdsEquivalent($0.id, selectedModelId) || workModelIdsEquivalent($0.modelId, selectedModelId) }) == false { + let matchedModelId = loadedModels.first( + where: { workModelIdsEquivalent($0.id, selectedModelId) || workModelIdsEquivalent($0.modelId, selectedModelId) } + )?.id + if let matchedModelId { + selectedModelId = matchedModelId + } + if resetSelection || matchedModelId == nil { if let preferred = loadedModels.first(where: \.isDefault) ?? loadedModels.first { selectedModelId = preferred.id selectedReasoningEffort = ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Work/WorkNewChatSheet.swift` around lines 400 - 408, The branch that preserves an equivalent selection must canonicalize selectedModelId to the loaded model's .id instead of leaving the old alias; update the block in WorkNewChatSheet.swift so that when loadedModels.contains(where: { workModelIdsEquivalent($0.id, selectedModelId) || workModelIdsEquivalent($0.modelId, selectedModelId) }) is true you find the matching model (e.g., first(where: { workModelIdsEquivalent($0.id, selectedModelId) || workModelIdsEquivalent($0.modelId, selectedModelId) }) ) and assign selectedModelId = matching.id (and clear selectedReasoningEffort as currently done); this mirrors the normalization in WorkSessionSettingsSheet+Actions.loadModels() and prevents submit() from sending a stale alias to createChatSession.apps/ios/ADE/Views/Components/ADEDesignSystem.swift (1)
132-137: ⚡ Quick winNormalize model-id lookups here too.
The rest of the iOS model flow now resolves ids through equivalence helpers, but these registries still depend on exact keys. A persisted or host-supplied alias like
openai/gpt-5.5-codexwill now missmodelBrand/reasoningTierseven though the picker treats it as the same model. Consider normalizing through the shared model-id helper, or keep both aliases until the wire migration is complete.Also applies to: 192-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Components/ADEDesignSystem.swift` around lines 132 - 137, The model registry entries in ADEDesignSystem.swift use exact string keys like "openai/gpt-5.5" / "gpt-5.5" which will miss persisted/host aliases (e.g., "openai/gpt-5.5-codex") and thus fail to populate modelBrand and reasoningTiers; update the lookup to normalize incoming model ids through the shared model-id equivalence helper (the same helper used by the picker flow) before accessing the registry (or alternatively add normalized aliases for each key), ensuring functions/variables that read the registry (the ADEDesignSystem model registry accessors and the modelBrand/reasoningTiers resolution logic) use the normalized id for lookups.
🤖 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/headlessLinearServices.test.ts`:
- Around line 423-424: The test assertions expect the canonical model id but the
constant HEADLESS_MODEL_ID in headlessLinearServices.ts is still set to the
legacy alias "openai/gpt-5.5-codex"; change HEADLESS_MODEL_ID to the canonical
"openai/gpt-5.5" in the headlessLinearServices.ts file (update the constant
definition) so the service default no longer relies on alias normalization and
the tests (e.g., session.modelId assertions in headlessLinearServices.test.ts)
are accurate without indirection.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 2293-2297: The code replaces the live-thread Codex reasoning
effort with the previously requested session value; instead, persist the
runtime-confirmed value from response.reasoningEffort when available. In the
block that computes requestedReasoningEffort (using validateReasoningEffort and
normalizeReasoningEffort) update the assignment to
managed.session.reasoningEffort to prefer response.reasoningEffort (if present
and non-null/undefined) and fall back to requestedReasoningEffort or existing
reasoningEffort only otherwise, so the backend-coerced/aliased value from
response is stored for subsequent turns.
In `@apps/desktop/src/main/services/sync/syncHostService.ts`:
- Around line 217-239: pruneMobileCommandResultCache currently evicts in-flight
records (records with completedAtMs == null) which can drop dedupe entries for
active commands; update the function so TTL-based cleanup still removes expired
entries (using referenceMs = completedAtMs ?? acceptedAtMs) but when enforcing
MOBILE_COMMAND_RESULT_CACHE_MAX_ENTRIES only evict from the completed set (the
"completed" array sorted by completion/accept time) and do not iterate over or
delete entries from the inFlight set; if you need a hard bound for active
commands, implement rejection logic elsewhere when the count of in-flight
entries would exceed your desired limit instead of removing existing in-flight
records.
In `@apps/desktop/src/main/services/sync/syncPeerService.ts`:
- Around line 175-210: pendingOutboundChangeset (used in sendLocalChanges) is
connection-scoped but not cleared on disconnect, so stale batches can be resent
after reconnect to a different host; fix by explicitly clearing or invalidating
pendingOutboundChangeset when the socket is torn down and/or when a new
connection is established (e.g., in disconnectInternal and the connect/reconnect
handler), ensuring any connection-specific metadata (batchId or sentAtMs) is
reset so sendLocalChanges will not resend old batches on a new connection.
- Around line 319-333: In the "changeset_ack" case, after a successful ACK for
pendingOutboundChangeset, update the tracked remote DB version(s) instead of
only clearing pendingOutboundChangeset: set latestRemoteDbVersion and
connectionDraft.lastRemoteDbVersion to at least the acknowledged toDbVersion
(e.g., Math.max(current, payload.toDbVersion)) so the saved draft/status
reflects the newly-applied remote version; keep the existing update to
outboundLocalDbVersion and then clear pendingOutboundChangeset. Reference:
pendingOutboundChangeset, payload.toDbVersion, latestRemoteDbVersion,
connectionDraft.lastRemoteDbVersion.
In `@apps/desktop/src/main/services/usage/usageTrackingService.test.ts`:
- Around line 319-322: The test for mini OpenAI models currently asserts only
input pricing; update the "returns mini pricing for mini OpenAI models" test
(the call to resolveTokenPrice("gpt-5.4-mini") and the local variable price) to
also assert price.output equals the expected output rate from the
openai-mini.output mapping so regressions are caught; add a second expectation
for price.output alongside the existing expect(price.input) assertion.
In `@apps/desktop/src/main/services/usage/usageTrackingService.ts`:
- Around line 518-520: The current model-classification check using
lower.includes("mini") incorrectly matches names like "gemini"; update the check
in usageTrackingService (the block that returns TOKEN_PRICES entries) to only
match genuine "mini" model IDs — e.g., replace the broad lower.includes("mini")
with a stricter condition such as matching the exact token or prefix (e.g.,
check lower === "openai-mini" or lower.startsWith("openai-mini")) or use a
word-boundary regex (/\bmini\b/) so only true "mini" model IDs map to
TOKEN_PRICES["openai-mini"] and avoid misclassifying "gemini".
In `@apps/desktop/src/renderer/components/settings/ChatAppearancePreview.tsx`:
- Line 102: Replace the outdated model ID string "openai/gpt-5.5-codex" in
headlessLinearServices.ts (around the code that constructs model descriptors or
defaultModelId usage) with the new "openai/gpt-5.5" format or, better, reference
the shared default model descriptor constant used elsewhere (so the service uses
the canonical model ID). Also update tests that still assert or inject the old
ID—specifically AgentChatMessageList.test.tsx and
headlessLinearServices.test.ts—to use "gpt-5.5" / "openai/gpt-5.5" (or the same
default model descriptor) so modelId formatting is consistent across
ChatAppearancePreview, headlessLinearServices, and test fixtures.
In `@apps/desktop/src/renderer/components/terminals/TerminalView.tsx`:
- Around line 123-128: terminalWebglRendererEnabled currently defaults to WebGL
which is unsafe on Linux VMs; update the function (terminalWebglRendererEnabled)
to detect Linux (e.g. process.platform === 'linux' or navigator.userAgent
containing "Linux") and, if no explicit user setting exists in localStorage
(TERMINAL_RENDERER_STORAGE_KEY returns null), return false (use DOM) for Linux;
otherwise honor the stored value (value !== "dom"). Alternatively ensure your
Linux deployment docs/config set ADE_DISABLE_HARDWARE_ACCEL=1, but implement the
platform-specific fallback in TerminalView.tsx to be safe.
In `@apps/desktop/src/shared/modelProfiles.ts`:
- Around line 49-50: The lookup fails for persisted legacy Codex ids (e.g.,
"openai/gpt-5.4-codex", "openai/gpt-5.5-codex") because callers now emit
canonical ids like DEFAULT_CODEX_MODEL_ID but findModel() does an exact match
against ALL_MODELS; fix by canonicalizing legacy Codex ids before lookup: update
the findModel() call site (or findModel() itself) to detect legacy aliases
(match /-codex$/) and map them to the canonical id (e.g.,
DEFAULT_CODEX_MODEL_ID) or resolve via the modelRegistry alias map (use whatever
alias-to-canonical lookup exists in modelRegistry) so findModel(legacyId)
becomes findModel(canonicalId) and legacy entries resolve to the correct
ModelEntry.
In `@apps/desktop/src/shared/types/sync.ts`:
- Around line 342-361: The shared types currently make batchId optional, which
allows creating ACKs the handlers will ignore; update the contract by making
batchId required on both SyncChangesetBatchPayload and SyncChangesetAckPayload
(remove "?: string | null" and use "batchId: string") so handlers that correlate
via payload.batchId always receive it, and update any callers/tests to supply a
non-null batchId (or alternatively add and document a clear fallback/legacy path
in the handlers that maps missing batchId to a deterministic id and update
SyncChangesetAckPayload.error handling accordingly); ensure you change
references to SyncChangesetBatchPayload, SyncChangesetAckPayload and usages of
payload.batchId across the codebase to match the new required field.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 5438-5450: The success branch for handling changeset acks doesn't
clear the transient sync error, so after a transient failure the UI can keep
showing an error; inside the ack.ok branch (where pendingOutboundChangeset is
set to nil, clearPendingOutboundChangesetForActiveProject() is called,
advanceOutboundCursorForActiveProject(...) is invoked, and lastSyncAt is set)
also reset lastError to nil (or empty string) so the transient error is
reconciled; if your sync/error state is persisted, also call the existing
persistence method for sync status (or update whatever mechanism persists
lastError) after clearing it.
- Around line 1262-1276: The active-project switch persists the new project
id/root but doesn't reload the new project's outbound sync state, so
outboundLocalDbVersion and pendingOutboundChangeset can remain from the previous
project; fix by invoking resetOutboundCursorStateForActiveProject() immediately
after setting activeProjectId/activeProjectRootPath and
database.setActiveProjectId(projectId) in the same block (the method where
previousProjectId/nextProjectId are compared), ensuring the outbound cursor and
pending batch for the newly active project are loaded.
In `@apps/ios/ADE/Views/Work/WorkChatSessionView.swift`:
- Around line 653-656: The composer lost the insertion path for mention/command
picks when the surrounding view removed the old picker/token handoff; restore a
draft-insert channel so selections can write into the draft again by adding an
insertion callback or channel to WorkChatComposerTextField (e.g., an
onInsertDraft: (String, Range?) -> Void or a DraftInsertChannel object) and wire
the mention/token picker to call that handler with the selected text and caret
info; ensure WorkChatSessionView still passes the same draftState into the
composer but also provides the new insertion closure/channel so `@-mentions` and
/-commands can append/replace text in the draft state.
In `@apps/ios/ADE/Views/Work/WorkModelCatalog.swift`:
- Around line 428-449: The switch in workKnownModelDisplayName is missing the
bare "claude-opus-4-7-1m" alias for the 1M Opus variant; update the case that
returns "Claude Opus 4.7 1M" (the case currently listing "opus[1m]", "opus-1m",
"anthropic/claude-opus-4-7-1m", "claude-opus-4-7[1m]") to also include
"claude-opus-4-7-1m" so that trimmed/lowercased IDs from the curated catalog map
to the canonical label.
In `@apps/ios/ADE/Views/Work/WorkModelPickerSheet.swift`:
- Around line 559-571: The UI uses workModelIdsEquivalent for isSelected but
other places still compare model.id == currentModelId which causes inconsistent
selection state; update all comparisons to use workModelIdsEquivalent(model.id,
currentModelId) instead of raw equality (replace uses inside reasoningPills, any
onSelect/commit guard logic, and anywhere model.id == currentModelId is
consulted) so the pills, card selection, and commit/onSelect behavior treat
aliased IDs (e.g., "gpt-5.4" vs "openai/gpt-5.4") as equivalent.
In `@apps/ios/ADETests/ADETests.swift`:
- Around line 1502-1535: The test currently reuses the same in-memory
DatabaseService across simulated restarts; instead persist/reopen the database
between restarts so state truly round-trips. Change the setup to capture the
temp directory (e.g. let tempDir = makeTemporaryDirectory()), construct the
first database with makeProjectLaneForeignKeyDatabase(baseURL: tempDir), then
before creating restartedBeforeAck and again before restartedAfterAck close the
current database and re-open a new DatabaseService via
makeProjectLaneForeignKeyDatabase(baseURL: tempDir), then construct new
SyncService instances (SyncService(database: ...)) against those reopened
databases and call setActiveProjectForTesting/advanceOutboundCursorForTesting as
before; apply the same reopen pattern for the later block mentioned (lines
1555-1590).
- Around line 5112-5113: The two identical assertions call
workModelIdsEquivalent("gpt-5.5", "openai/gpt-5.5") twice; replace the second
duplicate with the symmetry check workModelIdsEquivalent("openai/gpt-5.5",
"gpt-5.5") so the test verifies equivalence in both directions (catching
one-sided normalization). Update the second XCTAssertTrue to call
workModelIdsEquivalent with the arguments reversed to ensure the function is
symmetric.
---
Outside diff comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 13744-13750: The resume path is passing
managed.session.reasoningEffort (and the fallback from readPersistedState())
directly into runtime.request("thread/resume"), which can replay stale/unknown
values; mirror the validation/normalization performed in startFreshCodexThread()
by normalizing/trimming/lowercasing and mapping reasoningEffort to allowed
values (or falling back to DEFAULT_REASONING_EFFORT) before building the resume
request. Locate the runtime.request("thread/resume") call where resumeResponse
is assigned and replace the inline
managed.session.reasoningEffort/readPersistedState(...) expression with the same
normalized value-producing logic used by startFreshCodexThread(); do the same
for the other resume occurrence that mirrors this pattern.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 1335-1343: The PendingOperation struct and outbound-sync storage
use only project/site keys and can mix state between different paired hosts; add
a stable host identity (e.g., hostId: String) to the PendingOperation model and
include that hostId when building the outbound-sync storage key, and update any
enqueue/dequeue and read/write logic (the code paths that serialize/deserialize
PendingOperation and construct outbound-sync keys) to persist and check hostId;
alternatively, detect host change and clear related queued-operation and
outbound-sync stores when the current paired host id differs from stored hostId
to prevent cross-host replay/suppression.
In `@apps/ios/ADE/Views/Work/WorkModelCatalog.swift`:
- Around line 313-314: workDeduplicatedModelOptions currently deduplicates by
the literal model.id which allows duplicates like "gpt-5.4" and "openai/gpt-5.4"
to both appear; change the dedupe key to the model's equivalence identifier
(e.g., model.equivalenceId / model.equivalenceKey or a normalized id derived
from workModelEquivalence logic) so options are collapsed by logical equivalence
rather than raw id, and when multiple implementations exist choose a
deterministic winner (prefer curated/provider-preferred or first-seen) so
workDeduplicatedModelOptions returns one entry per equivalence and the
downstream sort call (workModelSortOrder) operates on equivalence-aware results.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsx`:
- Around line 1204-1227: Replace the hard 550ms setTimeout with Vitest fake
timers to make the assertion deterministic: call vi.useFakeTimers() before
rendering the AgentChatPane test, render the component, advance timers (e.g.,
vi.advanceTimersByTime or vi.runAllTimers) instead of awaiting setTimeout, then
assert readTranscriptTail was not called and finally restore with
vi.useRealTimers(); target the test case that references readTranscriptTail,
AgentChatPane, hideSessionTabs and isTileActive to locate where to apply the
change.
In `@apps/ios/ADE/Views/Components/ADEDesignSystem.swift`:
- Around line 132-137: The model registry entries in ADEDesignSystem.swift use
exact string keys like "openai/gpt-5.5" / "gpt-5.5" which will miss
persisted/host aliases (e.g., "openai/gpt-5.5-codex") and thus fail to populate
modelBrand and reasoningTiers; update the lookup to normalize incoming model ids
through the shared model-id equivalence helper (the same helper used by the
picker flow) before accessing the registry (or alternatively add normalized
aliases for each key), ensuring functions/variables that read the registry (the
ADEDesignSystem model registry accessors and the modelBrand/reasoningTiers
resolution logic) use the normalized id for lookups.
In `@apps/ios/ADE/Views/Work/WorkNewChatSheet.swift`:
- Around line 400-408: The branch that preserves an equivalent selection must
canonicalize selectedModelId to the loaded model's .id instead of leaving the
old alias; update the block in WorkNewChatSheet.swift so that when
loadedModels.contains(where: { workModelIdsEquivalent($0.id, selectedModelId) ||
workModelIdsEquivalent($0.modelId, selectedModelId) }) is true you find the
matching model (e.g., first(where: { workModelIdsEquivalent($0.id,
selectedModelId) || workModelIdsEquivalent($0.modelId, selectedModelId) }) ) and
assign selectedModelId = matching.id (and clear selectedReasoningEffort as
currently done); this mirrors the normalization in
WorkSessionSettingsSheet+Actions.loadModels() and prevents submit() from sending
a stale alias to createChatSession.
🪄 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: de3b99c7-aaf2-4a02-b8e0-1cdf7087e498
⛔ Files ignored due to path filters (3)
docs/features/chat/agent-routing.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**
📒 Files selected for processing (70)
apps/ade-cli/src/headlessLinearServices.test.tsapps/desktop/src/main/services/ai/aiIntegrationService.test.tsapps/desktop/src/main/services/ai/aiIntegrationService.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/automations/automationService.test.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/droidModelsDiscovery.tsapps/desktop/src/main/services/config/projectConfigService.aiModeMigration.test.tsapps/desktop/src/main/services/cto/ctoStateService.tsapps/desktop/src/main/services/cto/ctoWorkerLifecycle.test.tsapps/desktop/src/main/services/missions/phaseEngine.test.tsapps/desktop/src/main/services/missions/phaseEngine.tsapps/desktop/src/main/services/orchestrator/coordinatorTools.test.tsapps/desktop/src/main/services/orchestrator/executionPolicy.test.tsapps/desktop/src/main/services/orchestrator/executionPolicy.tsapps/desktop/src/main/services/orchestrator/orchestratorContext.tsapps/desktop/src/main/services/orchestrator/promptInspector.test.tsapps/desktop/src/main/services/prs/prIssueResolution.test.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/review/reviewContextBuilder.test.tsapps/desktop/src/main/services/review/reviewService.test.tsapps/desktop/src/main/services/review/reviewService.tsapps/desktop/src/main/services/review/reviewTargetMaterializer.test.tsapps/desktop/src/main/services/sync/syncHostService.test.tsapps/desktop/src/main/services/sync/syncHostService.tsapps/desktop/src/main/services/sync/syncPeerService.tsapps/desktop/src/main/services/usage/usageTrackingService.test.tsapps/desktop/src/main/services/usage/usageTrackingService.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/chatTranscriptRows.test.tsapps/desktop/src/renderer/components/missions/CreateMissionDialog.tsxapps/desktop/src/renderer/components/missions/missionHelpers.tsapps/desktop/src/renderer/components/missions/missionPhaseDefaults.tsapps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsxapps/desktop/src/renderer/components/prs/shared/PrConvergencePanel.test.tsxapps/desktop/src/renderer/components/prs/shared/PrIssueResolverModal.test.tsxapps/desktop/src/renderer/components/review/ReviewPage.test.tsxapps/desktop/src/renderer/components/review/ReviewPage.tsxapps/desktop/src/renderer/components/settings/ChatAppearancePreview.tsxapps/desktop/src/renderer/components/terminals/TerminalView.test.tsxapps/desktop/src/renderer/components/terminals/TerminalView.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/shared/chatModelSwitching.test.tsapps/desktop/src/shared/modelProfiles.test.tsapps/desktop/src/shared/modelProfiles.tsapps/desktop/src/shared/modelRegistry.test.tsapps/desktop/src/shared/modelRegistry.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Components/ADEDesignSystem.swiftapps/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swiftapps/ios/ADE/Views/Work/WorkChatSessionView.swiftapps/ios/ADE/Views/Work/WorkModelCatalog.swiftapps/ios/ADE/Views/Work/WorkModelPickerSheet.swiftapps/ios/ADE/Views/Work/WorkNewChatScreen.swiftapps/ios/ADE/Views/Work/WorkNewChatSheet.swiftapps/ios/ADE/Views/Work/WorkSessionSettingsSheet+Actions.swiftapps/ios/ADE/Views/Work/WorkSessionSettingsSheet.swiftapps/ios/ADE/Views/Work/WorkTimelineHelpers.swiftapps/ios/ADETests/ADETests.swift
💤 Files with no reviewable changes (1)
- apps/desktop/src/main/services/chat/droidModelsDiscovery.ts
Comment on lines
653
to
656
| WorkChatComposerTextField( | ||
| draftState: draftState, | ||
| canCompose: canCompose, | ||
| pendingInsert: $pendingInsert | ||
| canCompose: canCompose | ||
| ) |
There was a problem hiding this comment.
Restore a draft-insert channel for the composer.
After this change the text field only owns draftState, and the old picker/token handoff is gone from the surrounding view. That leaves no remaining path for @ mention or / command selections to write back into the draft, so the insertion workflow disappears entirely.
🤖 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 653 - 656,
The composer lost the insertion path for mention/command picks when the
surrounding view removed the old picker/token handoff; restore a draft-insert
channel so selections can write into the draft again by adding an insertion
callback or channel to WorkChatComposerTextField (e.g., an onInsertDraft:
(String, Range?) -> Void or a DraftInsertChannel object) and wire the
mention/token picker to call that handler with the selected text and caret info;
ensure WorkChatSessionView still passes the same draftState into the composer
but also provides the new insertion closure/channel so `@-mentions` and /-commands
can append/replace text in the draft state.
Mobile connection fixes: desktop sync host/peer changeset ack with retry + command result cache, iOS SyncService rework, iOS chat UI (WorkNewChatScreen/WorkChatSessionView/model picker) updates, model registry consolidation. Includes terminal color-env handling and WebGL-by-default renderer. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
arul28
force-pushed
the
ade/mobile-connection-fixes-cf51a514
branch
from
236ead4 to
33f3343
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
apps/desktop/src/main/services/sync/syncHostService.test.ts (1)
1563-1602: ⚡ Quick winExercise the ledger reload path, not just the write path.
These assertions only prove the file is written; the replay still happens in the same
createSyncHostService()instance. IfloadPersistedCommandLedger()regresses, this test still passes. Recreating the host before the secondcmd-quick-runwould actually cover the restart-safe guarantee.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncHostService.test.ts` around lines 1563 - 1602, The test currently only verifies the ledger write path but not reload; to exercise loadPersistedCommandLedger() recreate the SyncHostService between the first write and the second replay: after asserting the ledger file contains "cmd-quick-run" and before sending the second command (requestId "cmd-quick-run-conflict"), tear down the existing service/client created by createSyncHostService(), then instantiate a fresh createSyncHostService() (and new client/ws/queue) so the service loads the persisted ledger from disk; then send the second command and assert the same mismatch behavior and that createSpy was not called again. Ensure you rewire any references used in the test (client, createSpy) to the new instance so the replay path uses loadPersistedCommandLedger().
🤖 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/orchestrator/orchestratorContext.ts`:
- Around line 1299-1305: The costTier for the profile with modelId
"gpt-5.3-codex" in the orchestratorContext profile object is inconsistent with
the shared registry; update the costTier value from "medium" to "high" so it
matches apps/desktop/src/shared/modelRegistry.ts (look for the profile that
contains provider: "codex", modelId: "gpt-5.3-codex" and change its costTier
field to "high").
In `@apps/desktop/src/main/services/sync/syncHostService.ts`:
- Around line 539-562: writePersistedCommandLedger is persisting every completed
entry from mobileCommandResultCache into sync-mobile-command-ledger.json (via
commandLedgerPath), which can leak sensitive plaintext results; restrict
persistence by filtering records to a tight safelist of non-sensitive,
idempotent mutation actions (e.g., create/update/delete command names) using a
constant like ALLOWED_PERSISTED_ACTIONS before building the commands array, or
alternatively write the ledger into the encrypted store API instead of
writeTextAtomic; apply the same filtering/encrypted-store change to the other
persistence spot referenced around sendResult and the block at lines 1931-1950
so only safe actions (or encrypted storage) are used, and keep
MOBILE_COMMAND_RESULT_CACHE_MAX_ENTRIES and sorting logic unchanged.
In `@apps/desktop/src/main/services/sync/syncPeerService.ts`:
- Around line 175-185: The sendLocalChanges path (function sendLocalChanges) and
the ACK handling block that increments retryCount on ok: false must cap retries:
introduce a MAX_CHANGESET_RETRIES constant and, whenever
pendingOutboundChangeset.retryCount >= MAX_CHANGESET_RETRIES, stop retrying by
clearing pendingOutboundChangeset, stop calling sendOutboundChangeset, and
surface an error (e.g., call or emit an existing error/reporting helper or
processLogger.error) so the caller can handle a permanent failure; apply this
same check in both the timeout branch inside sendLocalChanges and the
ACK-handling branch that currently increments retryCount on ok: false (the code
referencing pendingOutboundChangeset, pendingOutboundChangeset.sentAtMs,
retryCount, sendOutboundChangeset, and the ACK ok:false path).
In `@apps/desktop/src/renderer/components/terminals/TerminalView.tsx`:
- Around line 124-133: The current try/catch returns true on any error which
enables WebGL on Linux even when storage access fails; update the catch block in
TerminalView (the code referencing TERMINAL_RENDERER_STORAGE_KEY) to replicate
the platform/userAgent check used above: on error, read
window.navigator.platform and window.navigator.userAgent (lowercased) and return
false for Linux hosts (platform or userAgent includes "linux"), otherwise return
true—so Linux still falls back to the safe renderer when localStorage access
throws.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 5479-5516: The pending outbound changeset retry count is
incremented in handleChangesetAck (NACK path) and sendLocalChanges (resend loop)
but never capped; add a ceiling check (matching host-side limit) before
incrementing/resending and move to an error/reconnect path when exceeded: in
both handleChangesetAck and sendLocalChanges inspect pending.retryCount (or
pending.retryCount + 1 if you increment after checking) and if it would exceed
the maxRetries, clear pendingOutboundChangeset, persist that state via
persistPendingOutboundChangesetForActiveProject(pending), set lastError to a
meaningful message, and trigger the reconnect/error flow (e.g., call the same
logic used elsewhere to transition to reconnect or call
advanceOutboundCursorForActiveProject only on success); otherwise increment
pending.retryCount, update pending.sentAt, assign pendingOutboundChangeset and
call sendOutboundChangeset(pending) as before.
---
Nitpick comments:
In `@apps/desktop/src/main/services/sync/syncHostService.test.ts`:
- Around line 1563-1602: The test currently only verifies the ledger write path
but not reload; to exercise loadPersistedCommandLedger() recreate the
SyncHostService between the first write and the second replay: after asserting
the ledger file contains "cmd-quick-run" and before sending the second command
(requestId "cmd-quick-run-conflict"), tear down the existing service/client
created by createSyncHostService(), then instantiate a fresh
createSyncHostService() (and new client/ws/queue) so the service loads the
persisted ledger from disk; then send the second command and assert the same
mismatch behavior and that createSpy was not called again. Ensure you rewire any
references used in the test (client, createSpy) to the new instance so the
replay path uses loadPersistedCommandLedger().
🪄 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: 3a17cc3b-8180-4681-9159-372959e7c037
⛔ Files ignored due to path filters (3)
docs/features/chat/agent-routing.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**
📒 Files selected for processing (71)
apps/ade-cli/src/headlessLinearServices.test.tsapps/ade-cli/src/headlessLinearServices.tsapps/desktop/src/main/services/ai/aiIntegrationService.test.tsapps/desktop/src/main/services/ai/aiIntegrationService.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/automations/automationService.test.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/droidModelsDiscovery.tsapps/desktop/src/main/services/config/projectConfigService.aiModeMigration.test.tsapps/desktop/src/main/services/cto/ctoStateService.tsapps/desktop/src/main/services/cto/ctoWorkerLifecycle.test.tsapps/desktop/src/main/services/missions/phaseEngine.test.tsapps/desktop/src/main/services/missions/phaseEngine.tsapps/desktop/src/main/services/orchestrator/coordinatorTools.test.tsapps/desktop/src/main/services/orchestrator/executionPolicy.test.tsapps/desktop/src/main/services/orchestrator/executionPolicy.tsapps/desktop/src/main/services/orchestrator/orchestratorContext.tsapps/desktop/src/main/services/orchestrator/promptInspector.test.tsapps/desktop/src/main/services/prs/prIssueResolution.test.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/review/reviewContextBuilder.test.tsapps/desktop/src/main/services/review/reviewService.test.tsapps/desktop/src/main/services/review/reviewService.tsapps/desktop/src/main/services/review/reviewTargetMaterializer.test.tsapps/desktop/src/main/services/sync/syncHostService.test.tsapps/desktop/src/main/services/sync/syncHostService.tsapps/desktop/src/main/services/sync/syncPeerService.tsapps/desktop/src/main/services/usage/usageTrackingService.test.tsapps/desktop/src/main/services/usage/usageTrackingService.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/chatTranscriptRows.test.tsapps/desktop/src/renderer/components/missions/CreateMissionDialog.tsxapps/desktop/src/renderer/components/missions/missionHelpers.tsapps/desktop/src/renderer/components/missions/missionPhaseDefaults.tsapps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsxapps/desktop/src/renderer/components/prs/shared/PrConvergencePanel.test.tsxapps/desktop/src/renderer/components/prs/shared/PrIssueResolverModal.test.tsxapps/desktop/src/renderer/components/review/ReviewPage.test.tsxapps/desktop/src/renderer/components/review/ReviewPage.tsxapps/desktop/src/renderer/components/settings/ChatAppearancePreview.tsxapps/desktop/src/renderer/components/terminals/TerminalView.test.tsxapps/desktop/src/renderer/components/terminals/TerminalView.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/shared/chatModelSwitching.test.tsapps/desktop/src/shared/modelProfiles.test.tsapps/desktop/src/shared/modelProfiles.tsapps/desktop/src/shared/modelRegistry.test.tsapps/desktop/src/shared/modelRegistry.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Components/ADEDesignSystem.swiftapps/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swiftapps/ios/ADE/Views/Work/WorkChatSessionView.swiftapps/ios/ADE/Views/Work/WorkModelCatalog.swiftapps/ios/ADE/Views/Work/WorkModelPickerSheet.swiftapps/ios/ADE/Views/Work/WorkNewChatScreen.swiftapps/ios/ADE/Views/Work/WorkNewChatSheet.swiftapps/ios/ADE/Views/Work/WorkSessionSettingsSheet+Actions.swiftapps/ios/ADE/Views/Work/WorkSessionSettingsSheet.swiftapps/ios/ADE/Views/Work/WorkTimelineHelpers.swiftapps/ios/ADETests/ADETests.swift
💤 Files with no reviewable changes (1)
- apps/desktop/src/main/services/chat/droidModelsDiscovery.ts
✅ Files skipped from review due to trivial changes (27)
- apps/desktop/src/renderer/components/prs/shared/PrIssueResolverModal.test.tsx
- apps/desktop/src/renderer/components/settings/ChatAppearancePreview.tsx
- apps/desktop/src/main/services/review/reviewTargetMaterializer.test.ts
- apps/desktop/src/main/services/review/reviewService.ts
- apps/desktop/src/renderer/components/prs/shared/PrConvergencePanel.test.tsx
- apps/desktop/src/main/services/orchestrator/executionPolicy.ts
- apps/desktop/src/main/services/missions/phaseEngine.test.ts
- apps/desktop/src/renderer/components/missions/CreateMissionDialog.tsx
- apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts
- apps/ade-cli/src/headlessLinearServices.ts
- apps/desktop/src/main/services/orchestrator/promptInspector.test.ts
- apps/desktop/src/renderer/components/missions/missionPhaseDefaults.ts
- apps/desktop/src/main/services/ai/aiIntegrationService.ts
- apps/desktop/src/main/services/orchestrator/executionPolicy.test.ts
- apps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsx
- apps/desktop/src/main/services/cto/ctoWorkerLifecycle.test.ts
- apps/desktop/src/main/services/ai/aiIntegrationService.test.ts
- apps/desktop/src/renderer/components/chat/chatTranscriptRows.test.ts
- apps/ade-cli/src/headlessLinearServices.test.ts
- apps/desktop/src/main/services/config/projectConfigService.aiModeMigration.test.ts
- apps/desktop/src/shared/modelProfiles.test.ts
- apps/desktop/src/renderer/components/missions/missionHelpers.ts
- apps/desktop/src/main/services/review/reviewService.test.ts
- apps/desktop/src/renderer/components/review/ReviewPage.tsx
- apps/desktop/src/main/services/prs/prIssueResolution.test.ts
- apps/desktop/src/main/services/orchestrator/coordinatorTools.test.ts
- apps/desktop/src/renderer/components/review/ReviewPage.test.tsx
🚧 Files skipped from review as they are similar to previous changes (23)
- apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx
- apps/desktop/src/main/services/missions/phaseEngine.ts
- apps/desktop/src/main/services/review/reviewContextBuilder.test.ts
- apps/desktop/src/renderer/components/chat/AgentChatPane.test.ts
- apps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsx
- apps/desktop/src/main/services/usage/usageTrackingService.ts
- apps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsx
- apps/desktop/src/renderer/components/terminals/TerminalView.test.tsx
- apps/ios/ADE/Views/Work/WorkSessionSettingsSheet+Actions.swift
- apps/desktop/src/main/services/automations/automationService.test.ts
- apps/desktop/src/renderer/browserMock.ts
- apps/ios/ADE/Models/RemoteModels.swift
- apps/ios/ADE/Views/Work/WorkNewChatSheet.swift
- apps/desktop/src/main/services/pty/ptyService.test.ts
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
- apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx
- apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx
- apps/ios/ADE/Views/Components/ADEDesignSystem.swift
- apps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsx
- apps/ios/ADETests/ADETests.swift
- apps/desktop/src/shared/chatModelSwitching.test.ts
- apps/desktop/src/main/services/cto/ctoStateService.ts
- apps/desktop/src/shared/modelRegistry.test.ts
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.
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Improvements
Greptile Summary
This PR adds a changeset ACK protocol (desktop ↔ iOS), a per-instance mobile command dedup/replay ledger, persisted outbound sync cursor on iOS, WebGL-by-default terminal heuristic, staggered tile transcript hydration, and normalised OpenAI model IDs that retire the
-codexsuffix variants while keeping them as backward-compat aliases.Confidence Score: 5/5
Safe to merge; all findings are P2 and do not block correctness.
All issues found are P2: an unbounded iOS model cache, silent swallow of foreground refresh errors, and a 10 s retry delay after explicit ACK failure. None cause data loss or broken sync in normal usage. The backward-compat concerns from the previous review round have been addressed in this PR.
apps/ios/ADE/Services/SyncService.swift — chatModelsCache growth and foreground refresh error handling
Important Files Changed
sendRequiredbypass for backpressure on critical messages.pendingOutboundChangeseton disconnect. Minor: explicitok: falseACK triggers a 10 s retry delay instead of an immediate resend.chatModelsin-flight dedup and caching, foreground refresh restore. Two P2 issues:chatModelsCacheis unbounded and foreground refresh errors are silently discarded.SyncChangesetAckPayloadand customDecodableinit forSyncChangesetBatchPayloadthat synthesises a legacybatchIdwhen absent, preserving backward compat with pre-PR desktop peers.batchIdtoSyncChangesetBatchPayload, newSyncChangesetAckPayloadtype,capabilitiesonSyncPeerMetadata, andchangesetAckfeature flag.-codexIDs as aliases, increases context windows forgpt-5.5andgpt-5.4to 1 M, removes older model entries.resolveCodexReasoningEffortForRuntimehelper, passesreasoningEffortat thread start/resume.isTileVisibleprop for transcript hydration of visible-but-unfocused tiles; staggered delay prevents thundering-herd on tab switch.@-mention and/-slash-command picker sheets from the iOS composer, simplifying the composition flow as intended.Sequence Diagram
sequenceDiagram participant iOS as iOS SyncService participant Desktop as Desktop syncHostService Note over iOS,Desktop: Outbound changeset (iOS → Desktop) iOS->>Desktop: changeset_batch (batchId, changes) alt apply success Desktop-->>iOS: changeset_ack (ok: true) iOS->>iOS: advanceOutboundCursor, clearPending else apply failure Desktop-->>iOS: changeset_ack (ok: false) iOS->>iOS: retryCount++, sentAt=now (10s delay) end Note over iOS,Desktop: Inbound changeset (Desktop → iOS) Desktop->>iOS: changeset_batch (batchId, changes) iOS->>iOS: applyChanges() iOS-->>Desktop: changeset_ack (ok: true/false) alt ack received and ok Desktop->>Desktop: advance lastKnownServerDbVersion, clear pending else ack timeout / repeated failure Desktop->>iOS: ws.close(4000) end Note over iOS,Desktop: Command dedup/replay iOS->>Desktop: command (commandId, action, args) Desktop->>Desktop: check mobileCommandResultCache alt cached result exists Desktop-->>iOS: command_ack (replay) Desktop-->>iOS: command_result (replay) else new command Desktop-->>iOS: command_ack (accepted) Desktop->>Desktop: execute command Desktop-->>iOS: command_result Desktop->>Desktop: persist to ledger endComments Outside Diff (2)
apps/desktop/src/main/services/sync/syncHostService.ts, line 351-363 (link)resendPendingChangesetBatchis called everyCHANGESET_ACK_TIMEOUT_MS(10 s) when a batch is unacknowledged, andretryCountis tracked but never acted upon — there is no circuit breaker or exponential back-off. A persistently failing peer will be retried indefinitely at 10 s intervals until the heartbeat eventually drops the connection. Consider capping retries (e.g. after N attempts log a warning and/or mark the peer as degraded) to bound resource usage and surface the issue faster.Prompt To Fix With AI
apps/ios/ADE/Services/SyncService.swift, line 588-599 (link)ISO8601DateFormatterinstantiated on every cursor/changeset persistBoth
persistOutboundCursorForActiveProjectandpersistPendingOutboundChangesetForActiveProjectcreate a newISO8601DateFormatter()inline.ISO8601DateFormatteris relatively expensive to allocate. Storing one as a lazily-initialised property onSyncService(similar to howdecoderis already a stored property) avoids repeated allocation on every sync tick.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (7): Last reviewed commit: "fix: gate changeset acknowledgements by ..." | Re-trigger Greptile