Huge Ade Mobile Pass#320
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Owner
Author
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughAdds Claude “auto” permission mode, derives initial-input session metadata, adjusts Codex initial input path, hardens sync/database ingestion, updates chat/terminal services, and overhauls multiple iOS/desktop UIs and helpers. Introduces notification fallbacks, clean shell spawning, pending-input tracking, and broader PR/queue/CTO command support. ChangesUnified CLI/Sync/UX Update
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Contributor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ios/ADE/Views/Work/WorkStatusAndFormattingHelpers.swift (1)
230-269:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
summary.statusis effectively ignored whenever a session exists.After Line 230, every recognized
session.status/runtimeStatepath returns before Lines 255-266 can run. That means a fresher chat summary can no longer flip a stale session row fromrunningtoidle/ended, and downstream UI keeps treating it as active until the session projection catches up.
↕️ Minimal precedence fixprivate func rawWorkChatSessionStatus(session: TerminalSessionSummary?, summary: AgentChatSessionSummary?) -> String { if summary?.awaitingInput == true { return "awaiting-input" } + if let status = summary?.status.lowercased() { + switch status { + case "active", "running": + return "active" + case "idle", "paused": + return "idle" + case "ended", "completed", "failed", "interrupted": + return "ended" + default: + break + } + } if let session { let sessionStatus = session.status.lowercased() let runtimeState = session.runtimeState.lowercased() if sessionStatus == "awaiting-input" || sessionStatus == "awaiting_input" || runtimeState == "waiting-input" { return "awaiting-input" @@ - if let status = summary?.status.lowercased() { - switch status { - case "active", "running": - return "active" - case "idle", "paused": - return "idle" - case "ended", "completed", "failed", "interrupted": - return "ended" - default: - break - } - } - guard let session else { return "ended" } return session.status.lowercased() == "running" ? "active" : "ended" }🤖 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/Work/WorkStatusAndFormattingHelpers.swift` around lines 230 - 269, The logic currently returns based solely on session.status/runtimeState when a session exists, ignoring summary?.status; adjust the precedence so summary.status can override stale session state (e.g., if summary?.status indicates "idle" or "ended" when session appears "running", treat it as idle/ended). Locate the block using session, session.status, session.runtimeState and the later guard reading summary?.status and either (a) evaluate summary?.status immediately after computing normalized session/runtimeState and before returning, or (b) add conditional checks inside the existing switch/default branches to consult summary?.status (checking "active"/"running", "idle"/"paused", "ended"/"completed"/"failed"/"interrupted") and return the summary-derived state where it reflects a more progressed state than the session; ensure all comparisons use lowercased() like the current code.apps/ade-cli/src/adeRpcServer.ts (1)
223-230:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't advertise
autoon the default Codex path.Line 230 now publishes
"auto"as a validspawn_agent.permissionMode, but Line 6895 rejects that combination wheneverproviderfalls back to the schema default ("codex"). Tool callers that rely on the schema/defaults can now construct a guaranteed-invalid request. Either make the schema conditional onprovider, or keep"auto"out of the shared enum and document it as Claude-only.Also applies to: 6889-6900
🤖 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/adeRpcServer.ts` around lines 223 - 230, The schema for spawn_agent currently lists permissionMode with enum ["default","auto","plan","edit","full-auto","config-toml"] which advertises "auto" for all providers but the request validator rejects "auto" when provider defaults to "codex"; fix by removing "auto" from the shared permissionMode enum (the permissionMode property in the schema) or make the schema conditional so that "auto" is only allowed when provider === "claude" (i.e., update the provider/permissionMode validation logic used by the spawn_agent handler to either restrict the enum or add provider-based schema branching).
🧹 Nitpick comments (7)
apps/ios/ADE/Views/Work/WorkChatSessionView+Actions.swift (1)
17-17: 💤 Low valueClarify the rationale for increasing debounce delay to 220ms.
The delay was increased from 80ms to 220ms. While this reduces timeline rebuild frequency (improving performance), it may make the UI feel less responsive to rapid transcript updates.
Was this change driven by profiling data showing excessive rebuilds, or is 220ms an empirically determined sweet spot for this view? Documenting the reasoning would help future maintainers understand the tradeoff.
🤖 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/Work/WorkChatSessionView`+Actions.swift at line 17, The debounce delay increase at the Task.sleep(for: .milliseconds(220)) call needs an in-code rationale and a more maintainable implementation: replace the magic number with a named constant (e.g., DEBOUNCE_DELAY_MS) and add a short comment above the constant or the Task.sleep call that states whether 220ms came from profiling data (reference the profiling run/metric), was empirically chosen, or is a compromise to reduce rebuilds vs responsiveness; if profiling was used, cite the metric (e.g., rebuilds/sec or CPU) and date, otherwise note it as an empirical sweet spot and leave a TODO to re-evaluate with profiling.apps/desktop/src/main/services/state/kvDb.ts (1)
3994-3996: ⚡ Quick winCache table PK metadata during batch apply to avoid per-row PRAGMA lookups.
The current loop does a schema query for every incoming change, which can become a sync bottleneck on large batches.
💡 Proposed direction
- for (const rawChange of changes) { - const change = normalizeIncomingCrsqlChange(db, rawChange); + const pkCountByTable = new Map<string, number>(); + for (const rawChange of changes) { + const change = normalizeIncomingCrsqlChange(db, rawChange, pkCountByTable);// helper signature idea function normalizeIncomingCrsqlChange( db: DatabaseSyncType, change: CrsqlChangeRow, pkCountByTable: Map<string, number> ): CrsqlChangeRow { /* ... */ }🤖 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/state/kvDb.ts` around lines 3994 - 3996, The per-row PRAGMA/schema lookup happens inside normalizeIncomingCrsqlChange causing a query for every change; modify the batch apply to first compute and cache primary-key column counts by table (e.g. build a Map<string, number> pkCountByTable), then change normalizeIncomingCrsqlChange signature to accept this pkCountByTable and use it instead of performing per-row schema queries, and update the loop where runStatement is called to pass pkCountByTable into normalizeIncomingCrsqlChange so each change uses the cached metadata.apps/ios/ADE/Views/Files/FilesDetailScreen.swift (1)
26-27: ⚡ Quick winScope new
@Stateproperties asprivate.
lastHandledFilesDetailRevisionandlastFilesDetailReloadshould beprivateto keep view-local state encapsulated.As per coding guidelines,
apps/ios/**/*.swift: iOS Swift app — check for memory management, Swift conventions, and proper SwiftUI patterns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ios/ADE/Views/Files/FilesDetailScreen.swift` around lines 26 - 27, Make the two `@State` properties view-local by marking lastHandledFilesDetailRevision and lastFilesDetailReload as private; update their declarations (the `@State` vars in FilesDetailScreen.swift) to use private so the state is encapsulated within the view and follows SwiftUI/Swift conventions for memory/scope management.apps/ios/ADE/Views/Work/WorkNewChatScreen.swift (1)
773-810: 💤 Low valueDuplicate
compactChoiceChipimplementation.This function is identical to the one defined in
WorkNewChatScreenat lines 186-223. Extract it to a shared location or make the outer one accessible to the inner struct to eliminate duplication.🤖 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/Work/WorkNewChatScreen.swift` around lines 773 - 810, compactChoiceChip is duplicated inside WorkNewChatScreen; remove the inner duplicate and refactor to a single shared implementation by either (1) moving compactChoiceChip(title:systemImage:tint:isSelected:accessibilityPrefix:action:) into the outer WorkNewChatScreen scope so the inner struct can call the outer function, or (2) extracting it to a shared helper (e.g., a private fileprivate function or a small View type) that both locations reference; update all call sites to use that single symbol and delete the redundant definition.apps/ios/ADE/Views/Lanes/LaneDetailContentSections.swift (1)
267-283: ⚡ Quick winAvoid nesting a horizontal
ScrollViewinside the existing horizontal chip row.
statusRowalready scrolls horizontally, so wrapping the PR chips in another horizontalScrollViewinvites drag-gesture contention and awkward VoiceOver focus. Returning theHStackdirectly keeps the layout without same-axis nesting.As per coding guidelines, `apps/ios/**/*.swift`: iOS Swift app — check for memory management, Swift conventions, and proper SwiftUI patterns.♻️ Simplify the PR chip container
- ScrollView(.horizontal, showsIndicators: false) { - HStack(spacing: 6) { - ForEach(Array(linkedPullRequests.enumerated()), id: \.offset) { _, pr in - Button { - onOpenLinkedPullRequest(pr) - } label: { - LaneTypeBadge( - text: "#\(pr.githubPrNumber)", - tint: lanePullRequestTint(pr.state) - ) - } - .buttonStyle(.plain) - .accessibilityLabel(pr.title.isEmpty ? "Open PR \(pr.githubPrNumber)" : "Open \(pr.title)") - } - } - } + HStack(spacing: 6) { + ForEach(Array(linkedPullRequests.enumerated()), id: \.offset) { _, pr in + Button { + onOpenLinkedPullRequest(pr) + } label: { + LaneTypeBadge( + text: "#\(pr.githubPrNumber)", + tint: lanePullRequestTint(pr.state) + ) + } + .buttonStyle(.plain) + .accessibilityLabel(pr.title.isEmpty ? "Open PR \(pr.githubPrNumber)" : "Open \(pr.title)") + } + } .accessibilityLabel("\(linkedPullRequests.count) linked pull requests")🤖 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/Lanes/LaneDetailContentSections.swift` around lines 267 - 283, Remove the horizontal ScrollView wrapper around the PR chips to avoid same-axis scroll nesting: replace the ScrollView(.horizontal, ...) block with the existing HStack(spacing: 6) { ForEach(Array(linkedPullRequests.enumerated()), id: \.offset) { _, pr in ... } } and move the accessibilityLabel("\(linkedPullRequests.count) linked pull requests") onto that HStack (or remove it if already covered by parent), keeping the Button, onOpenLinkedPullRequest(pr) call, LaneTypeBadge and lanePullRequestTint(pr.state) intact.apps/ios/ADE/Views/PRs/PrFiltersCard.swift (1)
79-98: 💤 Low valueRemove unused
sortMenuLabelview.This view builder was the label for the previous
Menu-based dropdown which has been replaced with a horizontal chip selector. It's no longer referenced anywhere in the file.🧹 Proposed cleanup
- `@ViewBuilder` - private var sortMenuLabel: some View { - HStack(spacing: 5) { - Image(systemName: "arrow.up.arrow.down") - .font(.system(size: 9, weight: .bold)) - Text(sortOption.title) - .font(.system(size: 11, weight: .semibold)) - } - .foregroundStyle(PrsGlass.textSecondary) - .padding(.horizontal, 10) - .padding(.vertical, 6) - .background( - Capsule(style: .continuous) - .fill(Color.white.opacity(0.05)) - ) - .overlay( - Capsule(style: .continuous) - .stroke(Color.white.opacity(0.10), lineWidth: 0.6) - ) - }🤖 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/PRs/PrFiltersCard.swift` around lines 79 - 98, The private ViewBuilder property `sortMenuLabel` is dead code (no longer referenced after replacing the Menu with the chip selector); remove the entire `@ViewBuilder private var sortMenuLabel: some View { ... }` declaration to clean up, ensure there are no remaining references to `sortMenuLabel` (and delete any now-unused supporting code tied only to it), then build the app to confirm no compiler warnings/errors remain.apps/ios/ADETests/ADETests.swift (1)
3691-3697: ⚡ Quick winScope database-change observers to avoid cross-test notification noise.
Using
NotificationCenter.defaultwithobject: nilcan count unrelated.adeDatabaseDidChangeevents and make these assertions flaky.Suggested fix
- let token = NotificationCenter.default.addObserver( + let token = NotificationCenter.default.addObserver( forName: .adeDatabaseDidChange, - object: nil, + object: database, queue: nil ) { _ in notificationCount += 1 }Also applies to: 3847-3853
🤖 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/ADETests/ADETests.swift` around lines 3691 - 3697, The observer for .adeDatabaseDidChange is currently registered with object: nil which captures unrelated notifications across tests; update the NotificationCenter.default.addObserver call to scope it to the specific database instance under test (pass that instance as the object instead of nil, e.g., the local db variable or the subject-under-test) so only changes from that instance increment notificationCount, and make the same change for the other occurrence noted (lines ~3847-3853). Keep the observer token name (token) and ensure you still remove the observer after the test (NotificationCenter.default.removeObserver(token)) to avoid leaks.
🤖 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/adeRpcServer.ts`:
- Around line 5254-5263: The metadata update is currently gated on
initialInputMeta.goal, causing promptTitle-derived titles to be treated as
manually named only when a goal exists; move the title-related update out of the
initialInputMeta.goal conditional so runtime.sessionService.updateMeta is
invoked for the title independently of goal. Specifically, keep the goal update
conditional (use session?.goal check and only include { goal:
initialInputMeta.goal } when appropriate) but always evaluate and include the
title/manuallyNamed piece when initialInputMeta.promptTitle exists and title ===
initialInputMeta.promptTitle, using the same keys ({ title:
initialInputMeta.promptTitle, manuallyNamed: false }) and still call
runtime.sessionService.updateMeta with the merged object for
created.sessionId/session variable.
In `@apps/ade-cli/src/services/sync/syncRemoteCommandService.ts`:
- Around line 1289-1305: The parser currently accepts any finite number for
confidenceThreshold; after const confidenceThreshold =
asOptionalNumber(value.confidenceThreshold) add a strict guard that rejects or
omits values outside the allowed range (e.g. require
Number.isFinite(confidenceThreshold) && confidenceThreshold >= 0 &&
confidenceThreshold <= 1); if the check fails throw a clear validation error or
do not include confidenceThreshold in the returned object so invalid numbers
like -1 or 1.5 cannot reach StartQueueAutomationArgs["confidenceThreshold"];
update any tests that exercise confidenceThreshold to expect this validation.
In `@apps/desktop/src/main/services/state/kvDb.ts`:
- Around line 739-753: normalizeIncomingCrsqlChange currently returns early for
byte-typed PKs via isSyncScalarBytes(packed) before verifying the table's
primary key arity, allowing malformed inputs to bypass validation; change the
logic to first query the table schema (using allRows and primaryKeyColumns from
the pragma) and validate primaryKeyColumns.length, then handle change.pk: if
primaryKeyColumns.length === 1 allow/convert scalar bytes via
packedCrsqlPrimaryKey(change.pk) (return change or {...change, pk: packedPk} as
appropriate), otherwise reject/non-scalar cases with an Error referencing
change.table and change.cid; preserve use of isSyncScalarBytes and
packedCrsqlPrimaryKey but ensure the table arity check happens before any early
return for byte-typed PKs.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 3752-3757: cachedChatModelCatalog() currently returns the newest
catalog from chatModelCatalogCache regardless of host/project scope; change it
to only consider entries whose cache key matches the current host/project scope
(use the same scoping logic as chatModelsCacheKey(provider:)), i.e. compute the
current cache key for the active provider/project and filter
chatModelCatalogCache to entries with that key before sorting by fetchedAt and
applying chatModelsCacheTTL, keeping the method name cachedChatModelCatalog(),
the cache variable chatModelCatalogCache, and the TTL constant
chatModelsCacheTTL intact.
- Around line 1655-1663: The current block in
refreshProjectCatalog(preferRemoteSelection:) can set activeProjectId to an ID
from deduplicatedRemoteProjectCatalog() that isn't present in the in-memory
projects list, causing activeProject to become nil; change the logic so that
before calling setActiveProjectId(remoteProject.id, rootPath: ... ) you verify
the chosen remoteProject.id exists in projects (e.g. projects.contains(where: {
$0.id == remoteProject.id })) or otherwise find the matching project from
projects by normalizedProjectRoot and use that project's id; only perform the
setActiveProjectId and return when the target ID is confirmed to be in projects
to avoid remapping to a deduped-away ID.
- Around line 7385-7414: In sendTestPush() in SyncService, short-circuit when
the socket is offline by checking canSendLiveRequests() before calling
awaitResponse; if canSendLiveRequests() is false, return a
SyncSendTestPushResult(ok: false, message: "The socket is offline" or similar)
immediately so you don't start awaitResponse/sendEnvelope and hang until
timeout; update the function to perform this pre-check (referencing
sendTestPush(), canSendLiveRequests(), awaitResponse(...), and
sendEnvelope(...)) and keep the existing result shapes for consistency.
In `@apps/ios/ADE/Views/Cto/CtoSettingsScreen.swift`:
- Around line 349-354: The current calls to syncService.fetchCtoBudget() and
syncService.fetchLinearConnectionStatus() only assign self.budget and
self.linearStatus on success, leaving stale values on failure; change the calls
to explicitly handle the failure case (e.g. use a do/catch or if let/else) and
reset self.budget and self.linearStatus to nil or a safe default in the
else/catch branch so stale UI state is cleared when fetches fail; locate the
assignments around syncService.fetchCtoBudget() and
syncService.fetchLinearConnectionStatus() and add the failure/reset handling for
self.budget and self.linearStatus.
In `@apps/ios/ADE/Views/Cto/CtoWorkerDetailScreen.swift`:
- Around line 529-535: When handling memoryResult in the switch, the failure
branch currently sets memoryLoadError but leaves coreMemory stale; update the
.failure(let err) branch to clear the previous snapshot by setting coreMemory =
nil (or an empty/default snapshot) in addition to setting memoryLoadError =
err.localizedDescription so stale memory is not shown when refresh fails; modify
the switch handling for memoryResult in CtoWorkerDetailScreen (the
.success/.failure branches) accordingly.
In `@apps/ios/ADE/Views/Lanes/LaneDetailContentSections.swift`:
- Around line 67-74: The current logic uses detail?.syncStatus fallback values
(syncStatus, remoteAhead, remoteBehind, hasUpstream, diverged) which causes the
Push button to be disabled while detail is loading; change the fallback to use
snapshot.lane.status as the temporary source of truth: when detail?.syncStatus
is nil, read ahead/behind/hasUpstream/diverged from snapshot.lane.status instead
of hardcoded defaults so shouldPush/shouldPull reflect the snapshot state during
initial render; apply the same replacement for the equivalent block around the
variables referenced on the other occurrence (the 98-104 region) so both
first-paint states use snapshot.lane.status until detail is available.
In `@apps/ios/ADE/Views/PRs/PrDetailChecksTab.swift`:
- Around line 10-21: The summary currently ignores .neutral in
prChecksSummaryStats (function prChecksSummaryStats, type PrCheck, helper
prCheckConclusionKind) but still counts neutrals in total, causing total >
pass+fail+pending; fix by treating .neutral as pass: increment pass in the
.neutral case (or alternatively add a neutral variable and include it in the
returned PrChecksSummaryStats if you prefer an explicit bucket) so that
pass+fail+pending (+neutral if added) always equals total; update the switch in
prChecksSummaryStats and the initializer/return (PrChecksSummaryStats)
accordingly to keep internal consistency.
In `@apps/ios/ADE/Views/PRs/PrDetailFilesTab.swift`:
- Around line 353-362: The inline action Buttons (prFileInlineAction) are
currently inside PrFileRowLabel which is used as the label for the outer
PrFileDiffCard Button, causing nested Button/tap conflicts; fix by removing the
HStack of prFileInlineAction from inside PrFileRowLabel and placing it outside
the outer Button (e.g., render the HStack as a sibling view alongside
PrFileDiffCard or in the trailing area of the row) so the outer Button only
wraps the row content and the action Buttons (which call onOpenFile and
onCopyPath) are independent Buttons not nested inside the label; update
PrFileRowLabel and PrFileDiffCard usages accordingly to ensure only the row
toggle is the outer Button and action Buttons are separate.
In `@apps/ios/ADE/Views/PRs/PrDetailOverviewTab.swift`:
- Around line 1674-1695: The inline action buttons (prInlineAction) render the
tappable area at 26×26 which is below the iOS 44pt accessibility touch target
(same issue exists in PrPathReviewCommentRow and PrIssueInventoryRow); update
the view so the button's hit area is at least 44×44 while keeping the icon
visually small: wrap the Image in the existing Button but set the Button (or its
container) to frame(width: 44, height: 44) and center the icon inside (or add
.contentShape(Circle()) / .padding to expand touch area) and keep the visual
circle/background sized for the icon, ensuring accessibilityLabel remains and
.buttonStyle(.plain) is preserved.
In `@apps/ios/ADE/Views/PRs/PrsRootScreen.swift`:
- Around line 1731-1737: The filter currently uses caseInsensitiveCompare, which
can match branches that only differ by case; change the comparison in the lanes
filter to a case-sensitive equality check by replacing
normalizedPrBranchName(lane.branchRef).caseInsensitiveCompare(expectedBranch) ==
.orderedSame with a direct case-sensitive comparison (e.g.
normalizedPrBranchName(lane.branchRef) == expectedBranch). Keep the existing
guards (lane.archivedAt, lane.laneType, and !expectedBranch.isEmpty) and update
only the equality check involving normalizedPrBranchName, lane.branchRef, and
expectedBranch.
- Around line 1855-1858: The current .onAppear uses item.linkedLaneId (and
fallback ids) without verifying they exist in availableLanes, which can leave
selectedLaneId set to an id not in the visible options; change the logic in the
.onAppear handler for selectedLaneId so it only assigns item.linkedLaneId or
exactBranchMatchedLane?.id if that id is contained in availableLanes.map { $0.id
} (or use availableLanes.contains(where: { $0.id == ... })), otherwise fall back
to availableLanes.first?.id or leave an empty string; update the selection
assignment near selectedLaneId/item.linkedLaneId/exactBranchMatchedLane to
perform these membership checks before setting selectedLaneId.
In `@apps/ios/ADE/Views/Work/WorkArtifactTerminalViews.swift`:
- Around line 291-304: The current submitReturn in WorkArtifactTerminalViews
sends buffered text first and then schedules a delayed "\r", which can reorder
or drop the return; change it to send the entire command and return in a single
terminal write via syncService.sendTerminalInput(sessionId: session.id, data:
input + "\r") (when input is non-empty) and, for the empty-input case, continue
to send just "\r"; also perform a connectionState check on syncService before
sending to avoid sending when disconnected and remove the Task sleep/delayed
send logic in submitReturn.
In `@apps/ios/ADE/Views/Work/WorkSessionDestinationView.swift`:
- Around line 659-680: The early return on fallbackEntries prevents upgrading a
truncated fallback to the canonical transcript; change the guard so we only skip
reconciliation when there is a non-truncated fallback. Replace the current check
(guard fallbackEntries.isEmpty else { return }) with logic that allows entry
when fallbackEntries is present but truncated (e.g., add a Bool property
fallbackIsTruncated or compute combined char count vs. the 32_000 cap), and
update loadTranscript(to: forceRemote:preferLightweight:) to set
fallbackIsTruncated (or ensure the truncation check is available) when it fills
fallbackEntries so reconcileIdleCanonicalTranscriptIfNeeded() can proceed only
when fallbackEntries is non-empty AND not truncated (guard
!(fallbackEntries.nonEmpty && !fallbackIsTruncated) else { return }).
In `@apps/ios/ADETests/ADETests.swift`:
- Around line 66-76: The test constructs a local `service` but never binds it to
the global used by the router; set `SyncService.shared = service` (after saving
`previousShared` and before calling `DeepLinkRouter.shared.handle(...)`) so
`DeepLinkRouter` actions target the intended `SyncService` instance; the
existing `defer { SyncService.shared = previousShared }` will restore the
previous global after the test.
---
Outside diff comments:
In `@apps/ade-cli/src/adeRpcServer.ts`:
- Around line 223-230: The schema for spawn_agent currently lists permissionMode
with enum ["default","auto","plan","edit","full-auto","config-toml"] which
advertises "auto" for all providers but the request validator rejects "auto"
when provider defaults to "codex"; fix by removing "auto" from the shared
permissionMode enum (the permissionMode property in the schema) or make the
schema conditional so that "auto" is only allowed when provider === "claude"
(i.e., update the provider/permissionMode validation logic used by the
spawn_agent handler to either restrict the enum or add provider-based schema
branching).
In `@apps/ios/ADE/Views/Work/WorkStatusAndFormattingHelpers.swift`:
- Around line 230-269: The logic currently returns based solely on
session.status/runtimeState when a session exists, ignoring summary?.status;
adjust the precedence so summary.status can override stale session state (e.g.,
if summary?.status indicates "idle" or "ended" when session appears "running",
treat it as idle/ended). Locate the block using session, session.status,
session.runtimeState and the later guard reading summary?.status and either (a)
evaluate summary?.status immediately after computing normalized
session/runtimeState and before returning, or (b) add conditional checks inside
the existing switch/default branches to consult summary?.status (checking
"active"/"running", "idle"/"paused", "ended"/"completed"/"failed"/"interrupted")
and return the summary-derived state where it reflects a more progressed state
than the session; ensure all comparisons use lowercased() like the current code.
---
Nitpick comments:
In `@apps/desktop/src/main/services/state/kvDb.ts`:
- Around line 3994-3996: The per-row PRAGMA/schema lookup happens inside
normalizeIncomingCrsqlChange causing a query for every change; modify the batch
apply to first compute and cache primary-key column counts by table (e.g. build
a Map<string, number> pkCountByTable), then change normalizeIncomingCrsqlChange
signature to accept this pkCountByTable and use it instead of performing per-row
schema queries, and update the loop where runStatement is called to pass
pkCountByTable into normalizeIncomingCrsqlChange so each change uses the cached
metadata.
In `@apps/ios/ADE/Views/Files/FilesDetailScreen.swift`:
- Around line 26-27: Make the two `@State` properties view-local by marking
lastHandledFilesDetailRevision and lastFilesDetailReload as private; update
their declarations (the `@State` vars in FilesDetailScreen.swift) to use private
so the state is encapsulated within the view and follows SwiftUI/Swift
conventions for memory/scope management.
In `@apps/ios/ADE/Views/Lanes/LaneDetailContentSections.swift`:
- Around line 267-283: Remove the horizontal ScrollView wrapper around the PR
chips to avoid same-axis scroll nesting: replace the ScrollView(.horizontal,
...) block with the existing HStack(spacing: 6) {
ForEach(Array(linkedPullRequests.enumerated()), id: \.offset) { _, pr in ... } }
and move the accessibilityLabel("\(linkedPullRequests.count) linked pull
requests") onto that HStack (or remove it if already covered by parent), keeping
the Button, onOpenLinkedPullRequest(pr) call, LaneTypeBadge and
lanePullRequestTint(pr.state) intact.
In `@apps/ios/ADE/Views/PRs/PrFiltersCard.swift`:
- Around line 79-98: The private ViewBuilder property `sortMenuLabel` is dead
code (no longer referenced after replacing the Menu with the chip selector);
remove the entire `@ViewBuilder private var sortMenuLabel: some View { ... }`
declaration to clean up, ensure there are no remaining references to
`sortMenuLabel` (and delete any now-unused supporting code tied only to it),
then build the app to confirm no compiler warnings/errors remain.
In `@apps/ios/ADE/Views/Work/WorkChatSessionView`+Actions.swift:
- Line 17: The debounce delay increase at the Task.sleep(for:
.milliseconds(220)) call needs an in-code rationale and a more maintainable
implementation: replace the magic number with a named constant (e.g.,
DEBOUNCE_DELAY_MS) and add a short comment above the constant or the Task.sleep
call that states whether 220ms came from profiling data (reference the profiling
run/metric), was empirically chosen, or is a compromise to reduce rebuilds vs
responsiveness; if profiling was used, cite the metric (e.g., rebuilds/sec or
CPU) and date, otherwise note it as an empirical sweet spot and leave a TODO to
re-evaluate with profiling.
In `@apps/ios/ADE/Views/Work/WorkNewChatScreen.swift`:
- Around line 773-810: compactChoiceChip is duplicated inside WorkNewChatScreen;
remove the inner duplicate and refactor to a single shared implementation by
either (1) moving
compactChoiceChip(title:systemImage:tint:isSelected:accessibilityPrefix:action:)
into the outer WorkNewChatScreen scope so the inner struct can call the outer
function, or (2) extracting it to a shared helper (e.g., a private fileprivate
function or a small View type) that both locations reference; update all call
sites to use that single symbol and delete the redundant definition.
In `@apps/ios/ADETests/ADETests.swift`:
- Around line 3691-3697: The observer for .adeDatabaseDidChange is currently
registered with object: nil which captures unrelated notifications across tests;
update the NotificationCenter.default.addObserver call to scope it to the
specific database instance under test (pass that instance as the object instead
of nil, e.g., the local db variable or the subject-under-test) so only changes
from that instance increment notificationCount, and make the same change for the
other occurrence noted (lines ~3847-3853). Keep the observer token name (token)
and ensure you still remove the observer after the test
(NotificationCenter.default.removeObserver(token)) to avoid leaks.
🪄 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: 46bdfb2c-b9f4-4427-b978-b50bec32ecc8
⛔ Files ignored due to path filters (8)
apps/ios/ADE.xcodeproj/project.pbxprojis excluded by!**/*.xcodeproj/project.pbxprojdocs/features/chat/README.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/**docs/features/sync-and-multi-device/remote-commands.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/pty-and-processes.mdis excluded by!docs/**docs/perf/ios-mobile-action-inventory.mdis excluded by!docs/**
📒 Files selected for processing (115)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/git/gitOperationsService.test.tsapps/desktop/src/main/services/git/gitOperationsService.tsapps/desktop/src/main/services/notifications/notificationEventBus.test.tsapps/desktop/src/main/services/notifications/notificationEventBus.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/state/kvDb.sync.test.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/services/sync/deviceRegistryService.test.tsapps/desktop/src/main/services/sync/syncHostService.test.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/renderer/components/terminals/cliLaunch.test.tsapps/desktop/src/shared/cliLaunch.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/sessions.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/App/ContentView.swiftapps/ios/ADE/App/DeepLinkRouter.swiftapps/ios/ADE/Models/NotificationPreferences.swiftapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/Database.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Shared/ADESharedContainer.swiftapps/ios/ADE/Shared/ADESharedModels.swiftapps/ios/ADE/Views/AttentionDrawer/AttentionDrawerModel.swiftapps/ios/ADE/Views/AttentionDrawer/AttentionDrawerSheet.swiftapps/ios/ADE/Views/Components/ADEDesignSystem.swiftapps/ios/ADE/Views/Components/ADEMobilePrimitives.swiftapps/ios/ADE/Views/Components/ADEStreamingShimmer.swiftapps/ios/ADE/Views/Cto/CtoBriefEditor.swiftapps/ios/ADE/Views/Cto/CtoIdentityEditor.swiftapps/ios/ADE/Views/Cto/CtoReloadHelpers.swiftapps/ios/ADE/Views/Cto/CtoRootScreen.swiftapps/ios/ADE/Views/Cto/CtoSessionDestinationView.swiftapps/ios/ADE/Views/Cto/CtoSettingsScreen.swiftapps/ios/ADE/Views/Cto/CtoTeamScreen.swiftapps/ios/ADE/Views/Cto/CtoWorkerDetailScreen.swiftapps/ios/ADE/Views/Cto/CtoWorkflowsScreen.swiftapps/ios/ADE/Views/Files/FilesDetailComponents.swiftapps/ios/ADE/Views/Files/FilesDetailScreen+Actions.swiftapps/ios/ADE/Views/Files/FilesDetailScreen.swiftapps/ios/ADE/Views/Files/FilesDirectoryContentsView+Actions.swiftapps/ios/ADE/Views/Files/FilesDirectoryContentsView.swiftapps/ios/ADE/Views/Files/FilesModels.swiftapps/ios/ADE/Views/Files/FilesRootComponents.swiftapps/ios/ADE/Views/Files/FilesRootScreen.swiftapps/ios/ADE/Views/Lanes/LaneBatchManageSheet.swiftapps/ios/ADE/Views/Lanes/LaneBranchPickerSheet.swiftapps/ios/ADE/Views/Lanes/LaneChatLaunchSheet.swiftapps/ios/ADE/Views/Lanes/LaneCommitHistoryScreen.swiftapps/ios/ADE/Views/Lanes/LaneCommitSheet.swiftapps/ios/ADE/Views/Lanes/LaneComponents.swiftapps/ios/ADE/Views/Lanes/LaneCreateSheet.swiftapps/ios/ADE/Views/Lanes/LaneDetailContentSections.swiftapps/ios/ADE/Views/Lanes/LaneDetailScreen.swiftapps/ios/ADE/Views/Lanes/LaneDiffScreen.swiftapps/ios/ADE/Views/Lanes/LaneFileTreeComponents.swiftapps/ios/ADE/Views/Lanes/LaneHelpers.swiftapps/ios/ADE/Views/Lanes/LaneListViewParts.swiftapps/ios/ADE/Views/Lanes/LaneManageSheet.swiftapps/ios/ADE/Views/Lanes/LaneTypes.swiftapps/ios/ADE/Views/LanesTabView.swiftapps/ios/ADE/Views/PRs/CreatePrWizardView.swiftapps/ios/ADE/Views/PRs/PrAiResolverCtaCard.swiftapps/ios/ADE/Views/PRs/PrDetailActivityTab.swiftapps/ios/ADE/Views/PRs/PrDetailChecksTab.swiftapps/ios/ADE/Views/PRs/PrDetailFilesTab.swiftapps/ios/ADE/Views/PRs/PrDetailOverviewTab.swiftapps/ios/ADE/Views/PRs/PrDetailScreen.swiftapps/ios/ADE/Views/PRs/PrFiltersCard.swiftapps/ios/ADE/Views/PRs/PrHelpers.swiftapps/ios/ADE/Views/PRs/PrMergeGateCard.swiftapps/ios/ADE/Views/PRs/PrWorkflowCards.swiftapps/ios/ADE/Views/PRs/PrsRootScreen.swiftapps/ios/ADE/Views/Settings/ConnectionSettingsView.swiftapps/ios/ADE/Views/Settings/NotificationsCenterView.swiftapps/ios/ADE/Views/Settings/PerSessionOverrideView.swiftapps/ios/ADE/Views/Settings/SettingsConnectionHeader.swiftapps/ios/ADE/Views/Settings/SettingsDiagnosticsSection.swiftapps/ios/ADE/Views/Settings/SettingsNotificationsSection.swiftapps/ios/ADE/Views/Settings/SettingsPairingSection.swiftapps/ios/ADE/Views/Work/WorkArtifactTerminalViews.swiftapps/ios/ADE/Views/Work/WorkBrowserHelpers.swiftapps/ios/ADE/Views/Work/WorkChatComposerAndInputViews.swiftapps/ios/ADE/Views/Work/WorkChatHeaderAndMessageViews.swiftapps/ios/ADE/Views/Work/WorkChatSessionView+Actions.swiftapps/ios/ADE/Views/Work/WorkChatSessionView+MessageLiveness.swiftapps/ios/ADE/Views/Work/WorkChatSessionView+Timeline.swiftapps/ios/ADE/Views/Work/WorkChatSessionView.swiftapps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swiftapps/ios/ADE/Views/Work/WorkNewChatScreen.swiftapps/ios/ADE/Views/Work/WorkNewChatSheet.swiftapps/ios/ADE/Views/Work/WorkPreviews.swiftapps/ios/ADE/Views/Work/WorkRootComponents.swiftapps/ios/ADE/Views/Work/WorkRootScreen+Actions.swiftapps/ios/ADE/Views/Work/WorkRootScreen+Selection.swiftapps/ios/ADE/Views/Work/WorkRootScreen.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView+Actions.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView.swiftapps/ios/ADE/Views/Work/WorkSessionGrouping.swiftapps/ios/ADE/Views/Work/WorkSessionSettingsSheet.swiftapps/ios/ADE/Views/Work/WorkStatusAndFormattingHelpers.swiftapps/ios/ADE/Views/Work/WorkTerminalEmulatorView.swiftapps/ios/ADE/Views/Work/WorkTimelineHelpers.swiftapps/ios/ADETests/ADETests.swiftapps/ios/ADETests/AttentionDrawerModelTests.swift
💤 Files with no reviewable changes (2)
- apps/ios/ADE/Views/Components/ADEStreamingShimmer.swift
- apps/ios/ADE/Views/Work/WorkChatSessionView+MessageLiveness.swift
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Owner
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses 20 actionable comments across TS (3) and iOS Swift (17): - ade-cli/adeRpcServer: decouple auto-title meta from goal - ade-cli/syncRemoteCommandService: validate confidenceThreshold 0..1 - desktop/kvDb: PK arity check runs before byte-typed PK accept - iOS Database: empty title skips title only, not whole update - iOS SyncService: contain activeProjectId remap; scope cached model catalog; fast-fail sendTestPush offline; indentation normalize - iOS Views Cto: clear stale budget/linearStatus/coreMemory on refresh failure - iOS Views Lanes: don't disable Push during sync-status loading - iOS Views PRs: neutral/skipped checks counted; 44pt touch targets; case-sensitive branch match; validate linkedLaneId against availableLanes - iOS Views Work: one-write terminal submit; don't skip idle transcript reconcile - iOS Tests: explicit SyncService.shared binding in deep-link test Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
arul28
force-pushed
the
ade/huge-ade-mobile-pass-831383b3
branch
from
9ff9c64 to
4aea07a
Compare
Owner
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
Release Notes
New Features
Improvements
UI Enhancements
Bug Fixes
Greptile Summary
This PR delivers a large batch of iOS mobile improvements alongside backend changes: queued/optimistic message steering, terminal output search, PR queue automation commands,
autopermission mode for Claude CLI sessions, and UX refinements across the Work, Lanes, and Settings tabs.archivedAt-aware session filtering, and a new work-session deep-link navigation request.autopermission mode,reasoningEffortpropagation to CLI session start, CRSQL primary-key normalisation for incoming sync deltas, and new PR queue/automation remote commands.sendTestPushupgraded to async with in-app fallback notification delivery, notification-preferences pruning on save, and per-table-schema caching improvements for outgoing CRDTs.Confidence Score: 4/5
Safe to merge with care — the main risk is terminal subscriptions accumulated silently during search, and the CRSQL batch abort on unsupported PK shapes (unchanged from a prior review).
The two issues found are non-blocking: terminal subscriptions from search are never cleaned up (ongoing data push after search clears) and
pragma table_infois queried once per incoming change row rather than once per table per batch. Neither breaks correctness, but both add avoidable load. All other new paths — steer fallback, in-app notification delivery, optimistic archive state, and CRSQL PK normalisation — look logically correct.apps/ios/ADE/Views/Work/WorkRootScreen+Actions.swift (terminal subscription cleanup) and apps/desktop/src/main/services/state/kvDb.ts (pragma table_info per-row cost)
Important Files Changed
pragma table_infois queried per-row without caching. The throw-on-unsupported-shape behaviour (flagged in a prior review) is unchanged.refreshTerminalSnapshotrefactor, in-app notification fallback via UNUserNotificationCenter, asyncsendTestPush, improved project selection ordering, chat-event deduplication before revision bump, and workspace-snapshot revision tracking.pending_input_item_idandarchived_atcolumns toterminal_sessions, consolidatedupdateSessionMeta, multi-project-scope artifact queries, and outgoing CRSQL PK encoding. Previous empty-title guard-early-return bug was addressed in this PR.hydrateSearchOutputBuffersIfNeededfor terminal output search,applyArchivedSessionOverridefor optimistic archive state, and navigation helpers. Terminal subscriptions created for search are never cleaned up when search is cleared.latestVisibleAssistantMessageIdfrom timeline presentation, and adds navigation/composer backdrop gradients.pendingInputItemIdcomputation for session summaries,includeArchivedfilter forlistSessions, and increasesMAX_TRANSCRIPT_READ_CHARSto 120k.autopermission mode,resolveCleanShellLaunchFieldsfor platform-aware clean shell launch,deriveTrackedCliInitialInputSessionMetafor auto-titling from initial prompt, andresolveCodexCliModelForLaunchto strip theopenai/vendor prefix.cto.getAgentCoreMemoryfrom heartbeat to agent service, makeslinearCredentialServiceoptional, addsreasoningEffortandmodelIdto CLI session start, and changeschat.sendtoawaitDispatch: false.pruningInactivePerSessionOverridesto drop no-op per-session entries before saving, keeping stored preferences lean.Sequence Diagram
sequenceDiagram participant iOS as iOS Client participant WS as WebSocket Sync participant Desktop as Desktop Host Note over iOS,Desktop: New: Send-to-Steer flow iOS->>WS: chat.send (awaitDispatch: false) WS-->>iOS: "{ok: true}" iOS->>iOS: updateLocalEcho(.sent) iOS->>WS: refreshChatState (forceRemote) alt Turn already active error iOS->>WS: chat.steer WS-->>iOS: "{ok: true, queued: true, steerId}" iOS->>iOS: upsertOptimisticPendingSteer(steerId) end Note over iOS,Desktop: New: In-app notification fallback Desktop->>WS: "in_app_notification {title, body, deepLink}" WS->>iOS: presentInAppNotification() iOS->>iOS: UNUserNotificationCenter.add(request) Note over iOS,Desktop: New: Work-session deep-link navigation Desktop->>WS: push WorkSessionNavigationRequest WS->>iOS: requestedWorkSessionNavigation published iOS->>iOS: handleRequestedWorkSessionNavigation() iOS->>iOS: NavigationPath.append(WorkSessionRoute)Comments Outside Diff (1)
apps/ios/ADE/Views/Work/WorkRootScreen.swift, line 93-114 (link)The old
archivedSessionIdslogic only included a locally-cached ID when the remote didn't know about the session (!remoteKnownIds.contains(id)). This ensured that once a remotechatSummariesentry appeared witharchivedAt == nil, the local storage fallback was suppressed. The newresolvedWorkArchivedSessionIdsunconditionally unions local storage with remote data, so any ID that remains inarchivedSessionIdsStorage— even one the remote explicitly unarchived — continues to mark the session as archived on this device.Concrete failure: user archives session X on iPhone A → iPhone B or the desktop unarchives it via
unarchiveChatSession→ iPhone A reloads and receives summaries witharchivedAt == nil, but itsAppStoragestill containssession.id. The new code returns that ID as archived; the old code filtered it out because the remote knew the session. iPhone A will continue hiding the session in the archived list until the user explicitly archives/unarchives it again on that device, clearing local storage.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "ship: iteration 3 — address coderabbit +..." | Re-trigger Greptile