ade code: per-provider Chat/CLI choice + TUI/remote/sync hardening pass#696
Conversation
Add an Interface: Chat | CLI row to the ADE Code (Ink TUI) new-chat and
/model setup panes, matching the desktop/iOS switcher. Chat creates an
SDK chat via chat.createSession (all providers, including Claude — a new
path); CLI starts a tracked provider CLI terminal via start_cli_session
(claude/codex/cursor/droid/opencode). Defaults to Chat; editable on a
draft, read-only once a session exists.
- Generalize the Claude-only terminal paths: startClaudeTerminalSession
-> provider-generic startCliTerminalSession; listTerminalSessions and
remoteLauncher.isTerminalSessionLaunchable now surface every tracked
CLI provider (not just Claude); terminalSessionToChatSummary, the
Ctrl+T control gates, TerminalPane status ("<PROVIDER> CONTROL"),
FooterControls label, and grid control hint are provider-neutral.
- Submit-path branching: focused terminal -> pty send/resume (Claude
keeps its double-enter, other providers use pty.sendToSession); draft
Interface=CLI -> tracked CLI terminal; otherwise chat.createSession.
- Interface-aware Cursor model gating in the model picker (Chat disables
CLI-only Cursor models and vice versa).
- Keep Claude-only chrome: closed-transcript stripping, naming hint, and
/model + /effort writing into a running Claude terminal.
- Tests: provider-generic start payloads (5 providers), trackedCli
provider resolution, listTerminalSessions/isTerminalSessionLaunchable
inclusivity, interface-row state machine + defaulting, Cursor gating.
- Docs: ADE Code README chat-setup + terminal-control sections.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…round refresh Findings: - H1: derive selectable transcript rows once per render and build copy text lazily. - H2: reuse pending steers, gate full Chat Info scans, and collapse cheap latest scans. - H3/M8: memoize hot TUI children and stabilize high-churn props without adding hover throttles. - H4: debounce/coalesce background session refreshes with an in-flight guard. - M5: LRU-cache assistant markdown parses by message text. - M7: tighten terminal grid reads and avoid trailing blank cell/row rendering. - L10: reconcile local Ink install to locked 7.1.0 and validate ADE CLI.
The in-project New Chat screen and the all-projects hub composer both defaulted the Chat/CLI switcher to .chat on every open, so the choice was forgotten across app restarts and project switches. Add a shared per-project store (WorkNewSessionModePreferences, keyed by project id in the app-group UserDefaults) that mirrors desktop's per-project WorkProjectViewState.draftKind. - Seed sessionMode from the store in each composer's init (not onAppear) so the sessionMode onChange never fires to reset runtimeMode to the provider default. - Persist only on an explicit Chat/CLI switcher tap via a new onUserSelect callback; programmatic model-availability fallbacks never write the store. - A stored CLI choice is honored only when the restored model can run in CLI; otherwise the session opens on chat without discarding the preference. Desktop already persists this per project (WorkProjectViewState.draftKind in localStorage ade.workViewState.v1, preserved across new-draft/close-tab/lane- refresh/project-switch); no desktop change needed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The hub composer seeded sessionMode once in init and never reloaded it when the destination project changed, so switching from a project saved as Chat to one saved as CLI (or vice versa) left submit branching on the stale mode. Centralize the availability-aware fallback in a pure WorkNewSessionModePreferences.resolvedMode(stored:modelId:provider:) (reused by both composers' init) and add HubComposerDrawer.reloadSessionMode(forProjectId:) called wherever pickedProjectId changes — the projectRow tap and the reconcileDestination fallback. Reload is read-only; explicit switcher taps remain the only writes, and a stored CLI choice still drops to chat for the session when the current model can't run in CLI without discarding the pref. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Re-review finding: exit 130/143 and runtimeState 'killed' are the daemon's
user-initiated close classification (ptyService.statusFromExit), so the
closed-session drawer was marking most intentionally closed CLI sessions
with the failed glyph. Only terminalStatus === 'failed' or a genuine
non-{0,130,143} exit code renders failed now.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR propagates runtime replay-gap metadata through buffering, RPC, IPC, and preload layers; hardens sync-host catalog delivery and backpressure handling; expands ADE CLI/TUI session, rendering, and model-selection flows; and adds per-project Chat/CLI session-mode persistence on iOS. ChangesRuntime gap/replay metadata
Sync host reliability and catalog delivery
ADE CLI and TUI client
iOS session mode preferences
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
arul28
changed the title
|
@copilot review but do not make fixes |
…ening branch Conflicts resolved: - apps/ios/ADE/Views/Hub/HubComposerDrawer.swift: re-wove per-project Chat/CLI interface persistence into #692's HubInlineComposer restructure (init seed capture + onUserSelect on the relocated switcher inside isExpanded). - docs/features/sync-and-multi-device/ios-companion.md: union of both maps. - brainProjectActionsSyncHandler.ts: pass crossProjectChatEnabled=false on the degraded fallback hello payload (new required field from #692; no foreign-chat provider on that path). Validated: ade-cli typecheck + sync/cli tests green; iOS build-for-testing green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@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: d03bd97152
ℹ️ 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".
| try { | ||
| const spawned = spawnDaemon(machineSocketPath); | ||
| const spawned = await withSocketSpawnLock(machineSocketPath, async () => { | ||
| if (fs.existsSync(machineSocketPath)) return false; |
There was a problem hiding this comment.
When the default machine socket path exists but no daemon is listening on it (for example after a crash leaves the Unix socket file behind), the first tryDaemon(1) fails and reaches this catch path, but the spawn lock returns false solely because the stale path still exists. The following tryDaemon(25) just retries the same dead socket and never launches a replacement daemon; before this change the catch path spawned unconditionally. Please remove/probe the stale socket before this guard, or allow spawnDaemon to run after a failed connect.
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/ios/ADE/Views/Hub/HubComposerDrawer.swift (1)
450-456: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid normalizing
sessionModeunconditionally on project switchesreloadSessionModestill feeds.onChange(of: sessionMode), andnormalizeSelection(for:)always ends by resettingruntimeModeto the provider default. Switching to a project with a different stored Chat/CLI mode can therefore overwrite a valid manual runtime choice even when the current model/provider is already allowed. Gate the fallback the same wayonAppearSetupdoes, or split the availability fallback from the default-reset path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ios/ADE/Views/Hub/HubComposerDrawer.swift` around lines 450 - 456, Avoid normalizing sessionMode unconditionally when switching projects in HubComposerDrawer. Update reloadSessionMode(forProjectId:) and normalizeSelection(for:) so that changing pickedProjectId/selectedLaneId only restores the project’s stored mode when it is still valid for the current provider/model, and does not always force runtimeMode back to the provider default. Match the gating already used by onAppearSetup, or separate the availability-fallback logic from the default-reset path so a valid manual Chat/CLI selection is preserved.apps/ade-cli/src/tuiClient/connection.ts (1)
942-959: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEmbedded mode still delivers gapped replay events; attached mode discards them.
In the attached path (lines 700-704), a detected gap clears
pendingand routes only throughonGap, skipping the (potentially incomplete) replayed events. Here in the embedded path,subscriptionArgs.onGap?.(gap)fires but the code still iteratesreplay.eventsand forwards them tocallbackunconditionally. A consumer switching between--embeddedand daemon-attached modes would see different behavior on the exact same gap scenario — one recovers cleanly viaonGap-triggered resync, the other silently receives a discontinuous replay through the normal path.As per coding guidelines, "For ADE CLI changes, verify both headless mode and the desktop socket-backed ADE RPC path" — please align these two code paths (either both forward pre-gap data or both suppress it), and add embedded-mode test coverage mirroring the existing attached-mode gap test.
🛠️ Proposed fix (align with attached-mode behavior)
const replay = subscriptionArgs.replay !== false && typeof eventBuffer.drain === "function" ? eventBuffer.drain(subscriptionArgs.cursor ?? 0, subscriptionArgs.limit ?? 100) : { events: [] }; const gap = runtimeEventGapMetadata(replay); - if (gap) subscriptionArgs.onGap?.(gap); - for (const event of replay.events) { + if (gap) { + subscriptionArgs.onGap?.(gap); + } else { + for (const event of replay.events) { if (shouldForward(event)) callback(event); } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/connection.ts` around lines 942 - 959, The embedded subscription path in subscribeRuntimeEvents forwards replay.events even after runtimeEventGapMetadata detects a gap, which makes it behave differently from the attached-mode gap handling. Update subscribeRuntimeEvents so that the replayed events are suppressed or otherwise handled consistently with the attached path when onGap is triggered, using the existing shouldForward/callback flow. Add embedded-mode coverage for the gap scenario to match the attached-mode test and verify both paths behave the same when a replay gap is detected.Source: Coding guidelines
apps/ade-cli/src/tuiClient/adeApi.ts (1)
79-100: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMisleading error when template lookup itself fails vs. template genuinely missing.
listTemplateserrors are swallowed to[]at line 85. If an explicittemplateIdis passed and the RPC call to list templates fails, the code throwsSetup template "X" was not found.even though the actual cause was an RPC failure, not template absence — this could confuse callers/users debugging setup failures.🩹 Proposed fix: distinguish RPC failure from missing template
- const [templates, defaultTemplateId] = await Promise.all([ - connection.action<LaneTemplate[]>("lane", "listTemplates").catch(() => []), - connection.action<string | null>("lane", "getDefaultTemplate").catch(() => null), - ]); + let templatesFetchFailed = false; + const [templates, defaultTemplateId] = await Promise.all([ + connection.action<LaneTemplate[]>("lane", "listTemplates").catch(() => { + templatesFetchFailed = true; + return []; + }), + connection.action<string | null>("lane", "getDefaultTemplate").catch(() => null), + ]); const explicitTemplateId = typeof options.templateId === "string" ? options.templateId.trim() : ""; const trimmedTemplateId = explicitTemplateId || (typeof defaultTemplateId === "string" ? defaultTemplateId.trim() : ""); const templateId = trimmedTemplateId ? templates.find((template) => template.id === trimmedTemplateId)?.id ?? null : null; if (explicitTemplateId && !templateId) { - throw new Error(`Setup template "${explicitTemplateId}" was not found.`); + throw new Error( + templatesFetchFailed + ? `Could not verify setup template "${explicitTemplateId}": failed to list templates.` + : `Setup template "${explicitTemplateId}" was not found.`, + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/adeApi.ts` around lines 79 - 100, runDefaultLaneSetup is masking failures from connection.action("lane", "listTemplates") by falling back to an empty array, which makes explicit template lookups throw a misleading “not found” error. Update the logic in runDefaultLaneSetup to distinguish RPC failure from a genuine missing template: capture errors from listTemplates separately, only report “Setup template … was not found” when templates were successfully loaded and the ID is absent, and otherwise surface the underlying RPC error before continuing to applyTemplate or initEnv.
🧹 Nitpick comments (10)
apps/ade-cli/src/services/sync/sharedSyncListener.ts (2)
179-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a regression test for the initial snapshot-overflow path.
The provided test coverage (
syncHostService.test.ts) exercises theonMessageoverflow path with a 600KB frame while parked, but this initial-buffering loop inpark()(executed when a snapshot already carries oversizedbufferedMessagesfrom handoff) has a separate/duplicated overflow branch that doesn't appear directly covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/services/sync/sharedSyncListener.ts` around lines 179 - 194, Add a regression test for the initial snapshot overflow path in sharedSyncListener’s park logic, since the existing syncHostService.test.ts coverage only hits the parked onMessage overflow case. Exercise the branch that iterates over snapshot.bufferedMessages in park(), provide an oversized preloaded bufferedMessages snapshot, and assert the websocket is closed with the handoff buffer exceeded code/message when PARKED_MESSAGE_BUFFER_LIMIT or PARKED_MESSAGE_BUFFER_BYTES is exceeded.
179-223: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate overflow-check logic between initial buffering and
onMessage.The count/byte threshold check (
bufferedMessages.length >= PARKED_MESSAGE_BUFFER_LIMIT || bufferedBytes(+entry.bufferedBytes) + messageBytes > PARKED_MESSAGE_BUFFER_BYTES) and the close(4002, ...) handling are copy-pasted between the initial loop (Lines 183-191) and theonMessagehandler (Lines 203-220). If one path's threshold or close reason is updated later, it's easy to miss the other, silently reintroducing an unbounded buffer path.Consider extracting a single helper, e.g.
tryBufferMessage(bufferedMessages, bufferedBytesRef, message), used by both call sites.♻️ Sketch of a shared helper
+function canBufferMore(currentCount: number, currentBytes: number, nextBytes: number): boolean { + return ( + currentCount < PARKED_MESSAGE_BUFFER_LIMIT && + currentBytes + nextBytes <= PARKED_MESSAGE_BUFFER_BYTES + ); +}Then both the initial loop and
onMessagecallcanBufferMore(...)instead of repeating the inline condition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/services/sync/sharedSyncListener.ts` around lines 179 - 223, The overflow check and close(4002, ...) handling are duplicated in the initial snapshot buffering loop and the onMessage handler, so extract that logic into a shared helper used by both paths. Centralize the buffer-limit/byte-limit check around PARKED_MESSAGE_BUFFER_LIMIT, PARKED_MESSAGE_BUFFER_BYTES, and entry.bufferedBytes so the behavior stays identical in one place. Keep the existing unpark and ws.close("Sync host handoff buffer exceeded") flow, but have both the snapshot replay loop and onMessage call the same helper to avoid drift.apps/desktop/src/shared/chatSubagents.ts (2)
310-363: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider splitting per-event-type handling into helpers.
The function mixes terminal-turn tracking,
teammate.idle,task.completed, andsubagent.*handling in one body, contributing to the flagged medium complexity. Extracting each branch into a small named helper (e.g.applyTeammateEvent,applySubagentEvent) would ease future maintenance as more event types/providers are added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/shared/chatSubagents.ts` around lines 310 - 363, The complexity in subagentActivitySummaryFromEvents comes from handling terminal turn tracking, teammate events, and subagent events all in one loop body. Refactor the branching logic into small named helpers such as applyTeammateEvent and applySubagentEvent (and a helper for terminal turn detection if helpful), then call them from subagentActivitySummaryFromEvents so each event type is handled in an isolated place while preserving the current snapshot and terminalTurnIds behavior.
365-379: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove dead
snapshots.setwrite-back.
snapshots.set(id, { ...snapshot, status })at line 376 has no effect on the returned value:totalCountusessnapshots.sizeandrunningCountis already accumulated from the localstatusvariable. The map is discarded right after the loop, so this mutation is inert and reads like leftover logic (perhaps copied from a sibling function that does return snapshots).♻️ Proposed cleanup
let runningCount = 0; - for (const [id, snapshot] of snapshots) { + for (const snapshot of snapshots.values()) { let status = snapshot.status; if ( snapshot.kind === "subagent" && status === "running" && snapshot.background !== true && snapshot.turnId && terminalTurnIds.has(snapshot.turnId) ) { status = "stopped"; - snapshots.set(id, { ...snapshot, status }); } if (status === "running") runningCount += 1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/shared/chatSubagents.ts` around lines 365 - 379, Remove the dead write-back inside the running-count loop in chatSubagents logic: in the snapshot iteration where status is switched from running to stopped for subagent terminal turns, delete the snapshots.set(id, { ...snapshot, status }) mutation because it does not affect the returned totals or any later result. Keep the local status adjustment in the loop that updates runningCount, and verify the relevant loop in chatSubagents is only using the local status variable rather than mutating the snapshots map.apps/ade-cli/src/tuiClient/format.ts (1)
387-464: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftStreaming deltas will pollute the markdown cache with one-off entries.
aggregate.tscallsparseAssistantMarkdown(mergedText)on every streamed delta merge, andmergedTextis a new/longer string on each call until the stream finishes. Each of these intermediate strings becomes its own cache entry that will (almost) never be looked up again, since it's superseded by the next delta's text. For a handful of concurrently streaming long messages, this can churn through the entire 500-entry/50MB budget with write-once garbage, evicting genuinely reusable entries for already-rendered messages and defeating the purpose of the cache for the case it seems intended to help (avoiding re-parse cost on scroll/re-render).Consider only caching "settled" text (e.g., gate caching on a completion signal from the caller, or key/cap based on whether the text is still actively growing) rather than caching every intermediate streaming state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/format.ts` around lines 387 - 464, The markdown cache in parseAssistantMarkdown is storing every intermediate streamed text snapshot, which creates one-off entries and churns the cache. Update parseAssistantMarkdown/rememberMarkdownParse so only settled text is cached, or add a caller-provided flag from aggregate.ts to skip caching while mergedText is still growing. Keep the reusable cache behavior for finalized messages by distinguishing streaming deltas from completed content.apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts (1)
1098-1151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame gap/oldestCursor normalization duplicated with
remoteConnectionPool.ts.
streamEventsForRoot(Lines 1130-1143) andnormalizeRuntimeEventsSubscribeResult(Lines 1847-1862) here are structurally identical tostreamEventsWithEntryandnormalizeRuntimeEventsSubscribeResultinapps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.ts(Lines 693-716 and 1157-1175). This is a good candidate to consolidate into a shared normalization utility used by both local and remote pools.Also applies to: 1836-1863
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts` around lines 1098 - 1151, The event-stream result normalization in streamEventsForRoot is duplicated in the remote pool and should be shared instead of copied. Extract the common gap/oldestCursor/eventEpoch/nextCursor normalization into a reusable helper and have both streamEventsForRoot and the corresponding remoteRuntime normalization path call it, using the existing streamEventsForRoot and normalizeRuntimeEventsSubscribeResult symbols to locate the duplicated logic.apps/desktop/src/main/services/ipc/runtimeBridge.ts (1)
1014-1021: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated gap/oldestCursor normalization pattern.
The
...(subscriptionInit?.gap === true ? { gap: true } : {})/oldestCursorconditional-spread block is repeated here (twice) and again almost verbatim inlocalRuntimeConnectionPool.tsandremoteConnectionPool.ts. Consider extracting a small shared helper (e.g.pickGapFields(init): Pick<RemoteRuntimeStreamEventsResult, "gap" | "oldestCursor">) to keep the normalization contract in one place.♻️ Example shared helper
function pickGapFields( source: Pick<RemoteRuntimeStreamEventsResult, "gap" | "oldestCursor"> | null, ): Pick<RemoteRuntimeStreamEventsResult, "gap" | "oldestCursor"> { return { ...(source?.gap === true ? { gap: true } : {}), ...(typeof source?.oldestCursor === "number" ? { oldestCursor: source.oldestCursor } : {}), }; }Also applies to: 1075-1082
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/ipc/runtimeBridge.ts` around lines 1014 - 1021, The `subscriptionInit` normalization for `gap` and `oldestCursor` is duplicated in `runtimeBridge.ts` and the same pattern is repeated in the connection pool code, so extract a small shared helper such as `pickGapFields` and use it from the relevant return-building paths. Update the `runtimeBridge` logic that constructs the result object to call the shared helper instead of inlining the conditional spreads, and reuse that helper in `localRuntimeConnectionPool` and `remoteConnectionPool` so the normalization contract lives in one place.apps/ade-cli/src/tuiClient/connection.ts (1)
852-858: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant conditional — both branches return the same call.
if (spawned) return await tryDaemon(25); return await tryDaemon(25);always resolves totryDaemon(25)regardless ofspawned. Compare to the analogous first-usage site (lines 836-842), which differentiates attempts viatryDaemon(spawned ? 25 : 1). Either this should collapse to a single unconditional call, or the differentiation was intended here too and was dropped by mistake — worth double-checking intent.♻️ Proposed simplification (if no differentiation intended)
const spawned = await withSocketSpawnLock(machineSocketPath, async () => { if (fs.existsSync(machineSocketPath)) return false; return spawnDaemon(machineSocketPath); }); - if (spawned) return await tryDaemon(25); - return await tryDaemon(25); + return await tryDaemon(25);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/connection.ts` around lines 852 - 858, The spawned check in connection.ts is redundant because both branches call the same tryDaemon(25) path. Update the logic around withSocketSpawnLock and spawnDaemon so it either collapses to a single unconditional tryDaemon(25) call, or, if the first-attempt behavior was meant to match the earlier tryDaemon(spawned ? 25 : 1) pattern, restore that intended differentiation using the spawned flag.apps/ade-cli/src/tuiClient/jsonRpcClient.ts (1)
260-268: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIsolate notification handlers so a throwing listener can't crash the socket.
handleDatacallshandleResponsein an unguarded loop; an exception from a notification handler propagates up into thesocket.on("data", …)callback and surfaces as an uncaught exception (process crash). Close handlers are already wrapped in try/catch (Lines 154-158) — mirror that here for consistency and resilience.🛡️ Proposed guard
if (typeof response.method === "string") { for (const handler of this.notificationHandlers.get(response.method) ?? []) { - handler(response.params); + try { + handler(response.params); + } catch { + // A notification listener throwing must not tear down frame parsing. + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/jsonRpcClient.ts` around lines 260 - 268, Notification handlers in handleResponse can throw and escape the socket data path, so wrap each handler invocation in try/catch inside JsonRpcClient.handleResponse, mirroring the existing close-handler guard used elsewhere in the class. Keep the logic around response.id and notificationHandlers.get(response.method) the same, but isolate each handler(response.params) call so one bad listener cannot crash the socket or terminate the process.apps/ade-cli/src/tuiClient/__tests__/Drawer.test.tsx (1)
119-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for untracked/chat-backed toolTypes.
None of the current cases pass a
toolTypelike"shell"or"codex-chat"throughderiveClosedCliSessions, so the misclassification issue flagged inclosedCliSessions.ts(provider fallback to"claude") isn't caught by this suite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/__tests__/Drawer.test.tsx` around lines 119 - 155, The Drawer test suite currently only covers terminal-backed closed sessions without exercising toolType-based classification, so it misses the provider fallback bug in deriveClosedCliSessions. Add coverage in Drawer.test.tsx for sessions passed through deriveClosedCliSessions with toolType values like "shell" and "codex-chat" to verify closedCliSessionStatusKind and closedCliRightPaneRow still classify them correctly. Use the existing terminal() helper and the deriveClosedCliSessions/closedCliSessionStatusKind symbols to locate the relevant assertions, and include a case that would fail if the provider defaults to "claude".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/eventBuffer.ts`:
- Around line 58-82: Track oversized event rejections in the EventBuffer gap
metadata by updating the logic in drainMetadata and push. Right now, oversized
events are skipped without affecting the gap calculation, so record the last
skipped BufferedEvent id when bytes exceeds maxEventBytes and include that
cursor in the gap check alongside retained events. Use the existing
drainMetadata and push helpers in eventBuffer.ts to keep oldestCursor and gap
accurate for replay consumers.
In `@apps/ade-cli/src/services/sync/syncHostService.ts`:
- Around line 1848-1869: Add a per-message timeout to the queued non-control
envelope handling in syncHostService’s ws.on("message") flow so a hung
handleMessage call cannot block peer.messageQueue forever. Update the
peer.messageQueue chain around handleMessage(peer, envelope) to race the handler
against a watchdog/abort, and treat timeout as a failed message. Keep the
existing parse and control-envelope handling intact, and make sure the timeout
path still logs through args.logger.warn with peer metadata.
In `@apps/ade-cli/src/tuiClient/components/TerminalPane.tsx`:
- Around line 384-391: The scan limit in TerminalPane’s row extraction is using
fallbackText.length, which counts UTF-16 code units instead of terminal columns
and can truncate wide-character lines. Update the scan loop in the TerminalPane
logic to iterate across the full terminal.cols range, and keep relying on
lastContentColumn plus the existing cells.length trim to remove trailing blanks.
In `@apps/ade-cli/src/tuiClient/connection.ts`:
- Around line 529-572: The lock handling in withSocketSpawnLock currently treats
any lock file older than 30s as stale based only on mtime, which can let an
active owner be replaced and then have its cleanup remove another process’s
lock. Update the lock ownership logic to write and verify an owner identity in
the lock file (for example process id plus timestamp) and only reclaim or unlink
the lock when the recorded owner matches the current holder; keep the timeout
behavior, but make the stale-lock check and finally cleanup in
withSocketSpawnLock and its lockPath/fd flow ownership-aware.
In `@apps/ade-cli/src/tuiClient/displayWidth.ts`:
- Around line 80-92: splitByDisplayCells is duplicating wide grapheme clusters
when a boundary falls inside a multi-cell character because it builds
before/selected/after with three independent sliceByDisplayCells calls. Fix
splitByDisplayCells to partition the string in a single pass over display
clusters so each cluster is assigned to exactly one segment, instead of letting
the same cluster satisfy overlap in multiple slices. Keep the behavior
consistent with sliceByDisplayCells and ensure the cursor/selection path used by
displayCellForCodeUnitIndex, app.tsx, and ChatView’s splitTextByColumns no
longer renders wide CJK/emoji characters twice.
In `@apps/ade-cli/src/tuiClient/modelState.ts`:
- Around line 362-374: Add explicit handling for the OpenCode-backed providers
in the permission helpers so Ollama and LM Studio do not fall through to Cursor
behavior. Update permissionSummary, permissionOptionsDetail, and
applyProviderPermissionMode to treat "ollama" and "lmstudio" the same way as
"opencode" by reusing the OpenCode permission mode mapping rather than
cursorModeLabel/cursorModeIdsForState or returning a no-op patch. Use the
existing provider mapping patterns from runtimeProviderForUiProvider and
fallbackModelStatePatch to keep the branches consistent across this file.
In `@apps/ade-cli/src/tuiClient/state.ts`:
- Around line 34-54: The final state write is fire-and-forget, so the shutdown
path can exit before the last save completes. Update the cleanup flow that calls
saveAdeCodeState/saveAdeCodeProjectState to await the async write (or add an
explicit flush before process exit), using saveAdeCodeStateAsync and the related
shutdown/heartbeat exit handling so the last state update is persisted.
In `@apps/desktop/src/preload/preload.ts`:
- Around line 1889-1893: The gap-notification path is triggering the same
project-binding change callback even when it’s for the current binding, which
makes renderer consumers like AppShell reapply project state unnecessarily.
Update the `batch.gap` handling in `preload.ts` so
`notifyProjectBindingChangedCallbacks(binding)` is only used for the intended
binding-change flow, and avoid treating same-binding gap events as a full
refresh; use `binding.key` in the callback path to keep the logic scoped. Also
remove `oldestCursor` from this desktop bridge path if it is not consumed
anywhere in the preload-to-renderer flow.
---
Outside diff comments:
In `@apps/ade-cli/src/tuiClient/adeApi.ts`:
- Around line 79-100: runDefaultLaneSetup is masking failures from
connection.action("lane", "listTemplates") by falling back to an empty array,
which makes explicit template lookups throw a misleading “not found” error.
Update the logic in runDefaultLaneSetup to distinguish RPC failure from a
genuine missing template: capture errors from listTemplates separately, only
report “Setup template … was not found” when templates were successfully loaded
and the ID is absent, and otherwise surface the underlying RPC error before
continuing to applyTemplate or initEnv.
In `@apps/ade-cli/src/tuiClient/connection.ts`:
- Around line 942-959: The embedded subscription path in subscribeRuntimeEvents
forwards replay.events even after runtimeEventGapMetadata detects a gap, which
makes it behave differently from the attached-mode gap handling. Update
subscribeRuntimeEvents so that the replayed events are suppressed or otherwise
handled consistently with the attached path when onGap is triggered, using the
existing shouldForward/callback flow. Add embedded-mode coverage for the gap
scenario to match the attached-mode test and verify both paths behave the same
when a replay gap is detected.
In `@apps/ios/ADE/Views/Hub/HubComposerDrawer.swift`:
- Around line 450-456: Avoid normalizing sessionMode unconditionally when
switching projects in HubComposerDrawer. Update reloadSessionMode(forProjectId:)
and normalizeSelection(for:) so that changing pickedProjectId/selectedLaneId
only restores the project’s stored mode when it is still valid for the current
provider/model, and does not always force runtimeMode back to the provider
default. Match the gating already used by onAppearSetup, or separate the
availability-fallback logic from the default-reset path so a valid manual
Chat/CLI selection is preserved.
---
Nitpick comments:
In `@apps/ade-cli/src/services/sync/sharedSyncListener.ts`:
- Around line 179-194: Add a regression test for the initial snapshot overflow
path in sharedSyncListener’s park logic, since the existing
syncHostService.test.ts coverage only hits the parked onMessage overflow case.
Exercise the branch that iterates over snapshot.bufferedMessages in park(),
provide an oversized preloaded bufferedMessages snapshot, and assert the
websocket is closed with the handoff buffer exceeded code/message when
PARKED_MESSAGE_BUFFER_LIMIT or PARKED_MESSAGE_BUFFER_BYTES is exceeded.
- Around line 179-223: The overflow check and close(4002, ...) handling are
duplicated in the initial snapshot buffering loop and the onMessage handler, so
extract that logic into a shared helper used by both paths. Centralize the
buffer-limit/byte-limit check around PARKED_MESSAGE_BUFFER_LIMIT,
PARKED_MESSAGE_BUFFER_BYTES, and entry.bufferedBytes so the behavior stays
identical in one place. Keep the existing unpark and ws.close("Sync host handoff
buffer exceeded") flow, but have both the snapshot replay loop and onMessage
call the same helper to avoid drift.
In `@apps/ade-cli/src/tuiClient/__tests__/Drawer.test.tsx`:
- Around line 119-155: The Drawer test suite currently only covers
terminal-backed closed sessions without exercising toolType-based
classification, so it misses the provider fallback bug in
deriveClosedCliSessions. Add coverage in Drawer.test.tsx for sessions passed
through deriveClosedCliSessions with toolType values like "shell" and
"codex-chat" to verify closedCliSessionStatusKind and closedCliRightPaneRow
still classify them correctly. Use the existing terminal() helper and the
deriveClosedCliSessions/closedCliSessionStatusKind symbols to locate the
relevant assertions, and include a case that would fail if the provider defaults
to "claude".
In `@apps/ade-cli/src/tuiClient/connection.ts`:
- Around line 852-858: The spawned check in connection.ts is redundant because
both branches call the same tryDaemon(25) path. Update the logic around
withSocketSpawnLock and spawnDaemon so it either collapses to a single
unconditional tryDaemon(25) call, or, if the first-attempt behavior was meant to
match the earlier tryDaemon(spawned ? 25 : 1) pattern, restore that intended
differentiation using the spawned flag.
In `@apps/ade-cli/src/tuiClient/format.ts`:
- Around line 387-464: The markdown cache in parseAssistantMarkdown is storing
every intermediate streamed text snapshot, which creates one-off entries and
churns the cache. Update parseAssistantMarkdown/rememberMarkdownParse so only
settled text is cached, or add a caller-provided flag from aggregate.ts to skip
caching while mergedText is still growing. Keep the reusable cache behavior for
finalized messages by distinguishing streaming deltas from completed content.
In `@apps/ade-cli/src/tuiClient/jsonRpcClient.ts`:
- Around line 260-268: Notification handlers in handleResponse can throw and
escape the socket data path, so wrap each handler invocation in try/catch inside
JsonRpcClient.handleResponse, mirroring the existing close-handler guard used
elsewhere in the class. Keep the logic around response.id and
notificationHandlers.get(response.method) the same, but isolate each
handler(response.params) call so one bad listener cannot crash the socket or
terminate the process.
In `@apps/desktop/src/main/services/ipc/runtimeBridge.ts`:
- Around line 1014-1021: The `subscriptionInit` normalization for `gap` and
`oldestCursor` is duplicated in `runtimeBridge.ts` and the same pattern is
repeated in the connection pool code, so extract a small shared helper such as
`pickGapFields` and use it from the relevant return-building paths. Update the
`runtimeBridge` logic that constructs the result object to call the shared
helper instead of inlining the conditional spreads, and reuse that helper in
`localRuntimeConnectionPool` and `remoteConnectionPool` so the normalization
contract lives in one place.
In `@apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts`:
- Around line 1098-1151: The event-stream result normalization in
streamEventsForRoot is duplicated in the remote pool and should be shared
instead of copied. Extract the common gap/oldestCursor/eventEpoch/nextCursor
normalization into a reusable helper and have both streamEventsForRoot and the
corresponding remoteRuntime normalization path call it, using the existing
streamEventsForRoot and normalizeRuntimeEventsSubscribeResult symbols to locate
the duplicated logic.
In `@apps/desktop/src/shared/chatSubagents.ts`:
- Around line 310-363: The complexity in subagentActivitySummaryFromEvents comes
from handling terminal turn tracking, teammate events, and subagent events all
in one loop body. Refactor the branching logic into small named helpers such as
applyTeammateEvent and applySubagentEvent (and a helper for terminal turn
detection if helpful), then call them from subagentActivitySummaryFromEvents so
each event type is handled in an isolated place while preserving the current
snapshot and terminalTurnIds behavior.
- Around line 365-379: Remove the dead write-back inside the running-count loop
in chatSubagents logic: in the snapshot iteration where status is switched from
running to stopped for subagent terminal turns, delete the snapshots.set(id, {
...snapshot, status }) mutation because it does not affect the returned totals
or any later result. Keep the local status adjustment in the loop that updates
runningCount, and verify the relevant loop in chatSubagents is only using the
local status variable rather than mutating the snapshots map.
🪄 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: 14642fb5-305d-4150-9bb3-608d5fbd7dbd
⛔ Files ignored due to path filters (7)
apps/ade-cli/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.jsondocs/ARCHITECTURE.mdis excluded by!docs/**docs/features/ade-code/README.mdis excluded by!docs/**docs/features/remote-runtime/README.mdis excluded by!docs/**docs/features/remote-runtime/internal-architecture.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 (78)
apps/ade-cli/README.mdapps/ade-cli/package.jsonapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/bootstrap.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/eventBuffer.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.tsapps/ade-cli/src/services/sync/sharedSyncListener.tsapps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/tuiClient/__tests__/ApprovalPrompt.test.tsxapps/ade-cli/src/tuiClient/__tests__/ChatView.test.tsxapps/ade-cli/src/tuiClient/__tests__/Drawer.test.tsxapps/ade-cli/src/tuiClient/__tests__/RightPane.test.tsxapps/ade-cli/src/tuiClient/__tests__/TerminalPane.test.tsxapps/ade-cli/src/tuiClient/__tests__/adeApi.test.tsapps/ade-cli/src/tuiClient/__tests__/aggregate.test.tsapps/ade-cli/src/tuiClient/__tests__/appInput.test.tsapps/ade-cli/src/tuiClient/__tests__/appPolling.test.tsxapps/ade-cli/src/tuiClient/__tests__/commands.test.tsapps/ade-cli/src/tuiClient/__tests__/connection.test.tsapps/ade-cli/src/tuiClient/__tests__/drawerLayout.test.tsapps/ade-cli/src/tuiClient/__tests__/format.test.tsapps/ade-cli/src/tuiClient/__tests__/keybindings.test.tsapps/ade-cli/src/tuiClient/__tests__/newLaneForm.test.tsapps/ade-cli/src/tuiClient/__tests__/pendingInput.test.tsapps/ade-cli/src/tuiClient/__tests__/planMode.test.tsapps/ade-cli/src/tuiClient/__tests__/remoteLauncher.test.tsapps/ade-cli/src/tuiClient/__tests__/state.test.tsapps/ade-cli/src/tuiClient/adeApi.tsapps/ade-cli/src/tuiClient/aggregate.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/bracketedPaste.tsapps/ade-cli/src/tuiClient/closedCliSessions.tsapps/ade-cli/src/tuiClient/commands.tsapps/ade-cli/src/tuiClient/components/ApprovalPrompt.tsxapps/ade-cli/src/tuiClient/components/ChatView.tsxapps/ade-cli/src/tuiClient/components/Drawer.tsxapps/ade-cli/src/tuiClient/components/FooterControls.tsxapps/ade-cli/src/tuiClient/components/Header.tsxapps/ade-cli/src/tuiClient/components/ModelPicker/ModelPickerPane.tsxapps/ade-cli/src/tuiClient/components/ModelPicker/modelPickerGeometry.tsapps/ade-cli/src/tuiClient/components/ModelPicker/modelPickerLayout.test.tsapps/ade-cli/src/tuiClient/components/ModelPicker/modelPickerLayout.tsapps/ade-cli/src/tuiClient/components/RightPane.tsxapps/ade-cli/src/tuiClient/components/TerminalPane.tsxapps/ade-cli/src/tuiClient/connection.tsapps/ade-cli/src/tuiClient/displayWidth.tsapps/ade-cli/src/tuiClient/drawerLayout.tsapps/ade-cli/src/tuiClient/drawerSelection.tsapps/ade-cli/src/tuiClient/format.tsapps/ade-cli/src/tuiClient/jsonRpcClient.tsapps/ade-cli/src/tuiClient/keybindings/index.tsapps/ade-cli/src/tuiClient/modelPickerController.tsapps/ade-cli/src/tuiClient/modelState.tsapps/ade-cli/src/tuiClient/newLaneForm.tsapps/ade-cli/src/tuiClient/pendingInput.tsapps/ade-cli/src/tuiClient/project.tsapps/ade-cli/src/tuiClient/providerMetadata.tsapps/ade-cli/src/tuiClient/relativeTime.tsapps/ade-cli/src/tuiClient/remoteBridge.tsapps/ade-cli/src/tuiClient/remoteLauncher.tsapps/ade-cli/src/tuiClient/state.tsapps/ade-cli/src/tuiClient/types.tsapps/ade-cli/tsup.config.tsapps/desktop/src/main/services/ipc/runtimeBridge.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/shared/chatSubagents.tsapps/desktop/src/shared/types/remoteRuntime.tsapps/ios/ADE/Views/Hub/HubComposerDrawer.swiftapps/ios/ADE/Views/Work/WorkNewChatScreen.swiftapps/ios/ADE/Views/Work/WorkPreviews.swiftapps/ios/ADE/Views/Work/WorkRootScreen.swiftapps/ios/ADETests/ADETests.swift
…, coderabbit x8) - remoteBridge + cli.ts transports: normalize JSON-RPC response ids (class sweep) - connection.ts: unlink stale socket before daemon spawn retry; owner-aware spawn-lock cleanup - eventBuffer: oversized skipped events mark replay gaps - syncHostService: queued-message watchdog timeout + warning metadata - TerminalPane: full-column wide-glyph scan - displayWidth: splitByDisplayCells single-pass grapheme partition (+ new test file) - modelState: ollama/lmstudio use OpenCode permission behavior (not Cursor) - state.ts: final state write flushed + awaited on signal exit - preload: gap polling no longer fires same-binding project refresh Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d80f5d981
ℹ️ 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".
| title, | ||
| terminalId, | ||
| cliLabel, | ||
| claudeChrome = true, |
There was a problem hiding this comment.
When a non-Claude CLI terminal is shown as a split/grid tile, the existing MultiChatGrid caller does not pass the new claudeChrome prop, so this default still runs compactClosedTerminalTranscript for closed Codex/Cursor/Droid/OpenCode transcripts. That cleaner removes Claude-specific prompt/footer/session lines, which can hide legitimate output from those providers in grid tiles; default to neutral cleanup or have the grid pass the derived terminal provider like the main pane does.
Useful? React with 👍 / 👎.
| export function truncateDisplayEnd(value: string, maxCells: number): string { | ||
| if (terminalDisplayWidth(value) <= maxCells) return value; | ||
| if (maxCells <= 1) return sliceByDisplayCells(value, 0, Math.max(0, maxCells)); | ||
| return `${sliceByDisplayCells(value, 0, maxCells - 1)}…`; |
There was a problem hiding this comment.
For labels beginning with a double-width grapheme, this path can return a string wider than maxCells: for example maxCells = 2 calls sliceByDisplayCells(..., 0, 1), which includes the whole 2-cell character, then appends a 1-cell ellipsis. Any chat footer/drawer cell using truncateDisplayEnd can overflow its allocated width for CJK or emoji text; reserve space in terminal cells before appending the ellipsis.
Useful? React with 👍 / 👎.
…ate overflow) - MultiChatGrid passes claudeChrome by derived terminal provider so non-Claude grid tiles keep neutral closed-transcript cleanup - truncateDisplayEnd uses a cluster-boundary-safe prefix so a leading wide grapheme can no longer overflow the allocated cell width (+ regression tests) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d442d2d627
ℹ️ 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".
| } | ||
|
|
||
| export function splitEditorCommand(value: string): string[] { | ||
| return value.trim().split(/\s+/).filter(Boolean); |
There was a problem hiding this comment.
When VISUAL/EDITOR contains shell quoting, such as a macOS app path with spaces or emacsclient -a "", this whitespace split turns the quoted segment into multiple argv entries and openKeybindingsFile() then spawns a non-existent command while swallowing the error. The previous shell-based path handled these editor commands, so users with quoted editor settings can no longer open the keybindings file; please parse quoted command strings or otherwise preserve quoted editor argv while still appending the target file safely.
Useful? React with 👍 / 👎.
splitEditorCommand now tokenizes with quote/escape support so editors like 'emacsclient -a ""' or app paths with spaces keep working after the shell:false hardening; quoted empty strings survive as argv entries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a50966c93
ℹ️ 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".
| const toolType = session.toolType ?? ""; | ||
| if (CHAT_BACKED_TERMINAL_TOOL_TYPES.has(toolType)) return false; | ||
| return isClaudeTerminalSession(session); | ||
| return trackedCliTerminalProvider(session) !== null; |
There was a problem hiding this comment.
When this broader terminal filter lets Codex/Cursor/Droid/OpenCode CLI sessions into the TUI, selecting one still hits the existing refresh path that treats every nextTerminalSession as Claude and applies fallbackModelStatePatch("claude") in app.tsx. In practice a newly started non-Claude CLI session refreshes with the wrong model/footer provider, so subsequent model changes or new CLI launches target Claude instead of the active provider. Hydrate the model state from terminalSessionProvider(nextTerminalSession) (or keep non-Claude terminals out until that path is provider-aware).
Useful? React with 👍 / 👎.
| function permissionOptionsDetail(modelState: AdeCodeModelState): string { | ||
| if (modelState.provider === "codex") return CODEX_PRESETS.join(" · "); | ||
| if (modelState.provider === "claude") return "default · plan · auto · bypass"; | ||
| if (runtimeProviderForUiProvider(modelState.provider) === "opencode") return OPENCODE_PERMISSION_OPTIONS.join(" · "); |
There was a problem hiding this comment.
For Ollama and LM Studio this row now advertises the OpenCode permission choices because runtimeProviderForUiProvider(...) === "opencode", but cyclePermission only updates opencodePermissionMode when modelState.provider === "opencode"; those local providers fall through to the Cursor-mode branch instead. When users choose Ollama/LM Studio and cycle Permissions, the visible OpenCode mode does not change and launches keep the old permission mode, so the advertised plan/full-auto/config-toml choices are unreachable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/ade-cli/src/tuiClient/connection.ts (1)
956-961: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRedundant branch: both paths call
tryDaemon(25).The
if (spawned)guard has no effect since both arms returntryDaemon(25). Compare with the initial-spawn path at Lines 944-945 which selects the attempt count viaspawned ? 25 : 1. Confirm this parity is intentional (i.e. we always want 25 attempts here even when no spawn occurred) rather than a copy from the other block that dropped the1-attempt case.♻️ Collapse to a single return if 25 is intended
- if (spawned) return await tryDaemon(25); - return await tryDaemon(25); + return await tryDaemon(spawned ? 25 : 1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/connection.ts` around lines 956 - 961, The `withSocketSpawnLock` result handling in `connection.ts` has a redundant `if (spawned)` branch because both paths return `tryDaemon(25)`. Update this block to either collapse it into a single return if 25 attempts is always intended, or restore the intended parity with the earlier spawn logic by varying the attempt count based on `spawned`. Use the `spawned`, `tryDaemon`, and `withSocketSpawnLock` symbols to locate and align this flow with the initial-spawn path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/ade-cli/src/tuiClient/connection.ts`:
- Around line 956-961: The `withSocketSpawnLock` result handling in
`connection.ts` has a redundant `if (spawned)` branch because both paths return
`tryDaemon(25)`. Update this block to either collapse it into a single return if
25 attempts is always intended, or restore the intended parity with the earlier
spawn logic by varying the attempt count based on `spawned`. Use the `spawned`,
`tryDaemon`, and `withSocketSpawnLock` symbols to locate and align this flow
with the initial-spawn path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32e06327-504d-4cf3-88f8-292097acb6fd
⛔ Files ignored due to path filters (3)
docs/ARCHITECTURE.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 (25)
apps/ade-cli/src/bootstrap.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/eventBuffer.tsapps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.tsapps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/tuiClient/__tests__/TerminalPane.test.tsxapps/ade-cli/src/tuiClient/__tests__/appInput.test.tsapps/ade-cli/src/tuiClient/__tests__/connection.test.tsapps/ade-cli/src/tuiClient/__tests__/displayWidth.test.tsapps/ade-cli/src/tuiClient/__tests__/keybindings.test.tsapps/ade-cli/src/tuiClient/__tests__/state.test.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/components/MultiChatGrid.tsxapps/ade-cli/src/tuiClient/components/TerminalPane.tsxapps/ade-cli/src/tuiClient/connection.tsapps/ade-cli/src/tuiClient/displayWidth.tsapps/ade-cli/src/tuiClient/keybindings/index.tsapps/ade-cli/src/tuiClient/modelState.tsapps/ade-cli/src/tuiClient/remoteBridge.tsapps/ade-cli/src/tuiClient/state.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/ios/ADE/Views/Hub/HubComposerDrawer.swiftapps/ios/ADETests/ADETests.swift
💤 Files with no reviewable changes (1)
- apps/desktop/src/preload/preload.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/ade-cli/src/tuiClient/tests/TerminalPane.test.tsx
- apps/ade-cli/src/tuiClient/tests/keybindings.test.ts
- apps/ade-cli/src/eventBuffer.ts
- apps/ade-cli/src/tuiClient/tests/state.test.ts
- apps/ios/ADETests/ADETests.swift
- apps/ade-cli/src/tuiClient/displayWidth.ts
- apps/ios/ADE/Views/Hub/HubComposerDrawer.swift
- apps/ade-cli/src/services/sync/syncHostService.test.ts
- apps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.ts
- apps/ade-cli/src/tuiClient/state.ts
- apps/ade-cli/src/tuiClient/modelState.ts
- apps/ade-cli/src/services/sync/syncHostService.ts
…permission cycle (codex P1/P2) - selecting a non-Claude tracked CLI session now hydrates model state from terminalSessionProvider instead of forcing claude (footer/model targets the right provider for subsequent changes and launches) - cyclePermission + updateChatModel payload route ollama/lmstudio through the OpenCode permission branch via runtimeProviderForUiProvider (class sweep) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Request the oversized catalog instead of sending empty projects
When the hello_ok envelope exceeds the catalog cap, this fallback sends an explicit empty projects array. The iOS hello handler treats any present projects field as the authoritative catalog and only sends project_catalog_request when that field is absent, so a host with enough projects to hit this cap causes a newly connected phone to apply an empty remote catalog after handshake even though the later request path can chunk the catalog. Please omit projects for the oversized case or immediately send the chunked catalog after hello.
ℹ️ 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".
1 participant
ADE Code hardening + per-provider Chat/CLI interface choice
Large hardening and feature pass on
ade code(TUI), plus remote/sync reliability and cross-surface interface persistence. 13 commits, audited → implemented → dual-reviewed → quality/test-gated.Feature: Chat vs CLI per provider (desktop/iOS parity)
Interface: Chat | CLIrow in the TUI new-chat/model setup panes. Chat creates an SDK chat viachat.createSession(all providers — Claude SDK chat creation from the TUI is new); CLI starts a tracked provider CLI terminal viastart_cli_session(claude/codex/cursor/droid/opencode).<PROVIDER> CONTROL), non-Claude tracked CLIs now listed/resumable everywhere incl.ade code remote.Reliability (remote/sync)
ade code remote: route re-selection on reconnect, bounded remote stderr surfaced in errors, child/socket cleanup, Unix-socket (0700) local bridge, version/capability warnings.Performance
React.memo+ stabilized props on hot children, 200ms debounced background refresh (foreground no longer stutters when other lanes stream), markdown LRU, terminal grid read tightening.Bugs + parity + UX
agimode,/loginretry prefill, Claude hook-error compaction,/new laneLinear+template fields, lane setup retry, durable draft-launch status, closed-session browsing (closed (N)toggle + resume),/secretsread/copy pane, digit-picker highlight-then-Enter semantics (free text can start with a digit), UI polish sweep.app.tsx−654 lines via pure-module extractions (modelState, closedCliSessions, modelPickerController, remoteBridge, providerMetadata).Verification
build-for-testinggreen.🤖 Generated with Claude Code
Summary by CodeRabbit
terminal list/lsenhancements andterminal resume(reattach) for provider CLI terminal sessions, including new “ended”/“resume” table columns./secretscommand and expanded model picker + new lane setup with template/Linear issue inputs and interface-aware behavior.Greptile Summary
This PR adds per-provider Chat/CLI selection across ADE Code and hardens remote and sync runtime paths. The main changes are:
Confidence Score: 5/5
This PR appears safe to merge from the reviewed changes.
No accepted bug findings remain. The reviewed high-risk paths include remote bridge framing and teardown, JSON-RPC timeouts, sync catalog/backpressure handling, TUI interface-aware launch logic, and iOS Chat/CLI persistence.
No files require special attention.
What T-Rex did
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant User participant UI as TUI / iOS Composer participant Runtime as ADE Runtime RPC participant Chat as SDK Chat Service participant Pty as Provider CLI Terminal participant Remote as Remote Bridge / Sync Host User->>UI: Select provider, model, and interface alt "Interface = Chat" UI->>Runtime: chat.createSession / chat.send Runtime->>Chat: Create SDK-backed session Chat-->>Runtime: Session and streamed events else "Interface = CLI" UI->>Runtime: work.startCliSession / pty.resumeSession Runtime->>Pty: Launch tracked provider CLI Pty-->>Runtime: Terminal session and transcript events end Runtime-->>UI: Runtime events and terminal snapshots opt Remote or sync client UI->>Remote: JSON-RPC or sync request Remote->>Runtime: Forward request with guards Runtime-->>Remote: Response, events, or catalog chunks Remote-->>UI: Framed response or bounded disconnect end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant User participant UI as TUI / iOS Composer participant Runtime as ADE Runtime RPC participant Chat as SDK Chat Service participant Pty as Provider CLI Terminal participant Remote as Remote Bridge / Sync Host User->>UI: Select provider, model, and interface alt "Interface = Chat" UI->>Runtime: chat.createSession / chat.send Runtime->>Chat: Create SDK-backed session Chat-->>Runtime: Session and streamed events else "Interface = CLI" UI->>Runtime: work.startCliSession / pty.resumeSession Runtime->>Pty: Launch tracked provider CLI Pty-->>Runtime: Terminal session and transcript events end Runtime-->>UI: Runtime events and terminal snapshots opt Remote or sync client UI->>Remote: JSON-RPC or sync request Remote->>Runtime: Forward request with guards Runtime-->>Remote: Response, events, or catalog chunks Remote-->>UI: Framed response or bounded disconnect endComments Outside Diff (1)
apps/ade-cli/src/tuiClient/adeApi.ts, line 529 (link)fastMode: falseis currently omitted, so a user who disables the fast toggle on a supported chat model does not send that choice tochat.createSession; the runtime can then apply its default instead. This contradicts the iOS path’s explicit true/false behavior and makes the TUI unable to honor an explicit OFF selection.Prompt To Fix With AI
Reviews (6): Last reviewed commit: "ship: iteration 4 — provider-aware termi..." | Re-trigger Greptile