Conversation
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds lane-list snapshots and snapshot-driven UI/state across desktop and iOS; large iOS lane UI feature suite; multi-column primary-key DB handling; SyncService lane APIs and queued-command error; embedding cache-clear API and tests; removes GitHub CLI support; and adds factory scripts and iOS docs/update commands. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Owner
Author
|
@codex review |
Owner
Author
|
@copilot review |
Contributor
Here's my review of the three commits in this PR: OverallGood set of changes. The refactor of Issues1.
|
![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: ae852fbcd8
ℹ️ 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".
| lanes: LaneSummary[], | ||
| ): Promise<LaneListSnapshot[]> { | ||
| const [sessions, rebaseSuggestions, autoRebaseStatuses, stateSnapshots, batchAssessment] = await Promise.all([ | ||
| Promise.resolve(args.sessionService.list({ limit: 500 })), |
There was a problem hiding this comment.
buildLaneListSnapshots reads sessions via sessionService.list(...), which bypasses the enrichment/filtering done in IPC.sessionsList (chat runtime overrides like waiting-input/idle, and identity-session filtering). In lanes that only have chat sessions awaiting input, this path will classify them as generic running/ended based on stale DB data, so runtime badges and attention-driven refresh behavior become incorrect compared with the existing session list semantics.
Useful? React with 👍 / 👎.
Comment on lines
+761
to
+762
| Promise.resolve(args.rebaseSuggestionService?.listSuggestions() ?? []), | ||
| Promise.resolve(args.autoRebaseService?.listStatuses() ?? []), |
There was a problem hiding this comment.
The new snapshot endpoint awaits rebaseSuggestionService.listSuggestions() and autoRebaseService.listStatuses() inside a single Promise.all without per-call fallbacks. If either auxiliary source throws (for example, transient git/DB failures), the whole lanes.listSnapshots IPC call rejects and the lane list cannot hydrate, even though the core lane list itself is still available. These metadata calls were previously best-effort in the renderer; they should remain non-fatal here.
Useful? React with 👍 / 👎.
Comment on lines
+2240
to
+2244
| let pkColumns = tableInfo.primaryKeyColumns | ||
| let decodedPkValues: [SyncScalarValue] | ||
| if pkColumns.count == 1 { | ||
| decodedPkValues = [decodeCrsqlPk(first.pk)] | ||
| } else if let multi = decodeCrsqlPkColumns(first.pk), multi.count == pkColumns.count { |
There was a problem hiding this comment.
This change adds multi-column PK decoding (pkColumns), but deletion inference still routes through rowChangesRepresentDeletedRow(..., primaryKeyColumn:), which excludes only the first PK column when comparing nullified columns. For composite-key tables, the remaining PK columns are incorrectly treated as non-key fields, so null-tombstone change sets can fail deletion detection and leave stale rows behind.
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: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/state/appStore.ts (1)
162-178:⚠️ Potential issue | 🟠 MajorIgnore stale
refreshLanes()responses.This store intentionally fires more than one lane refresh during hydration, but the only staleness check here is
projectKey. If the earlierincludeStatus: falserequest resolves after the later warm refresh, it will overwrite the richerlaneSnapshotsstate with older data for the same project. Add a monotonic request token/version and drop out-of-order completions.Also applies to: 280-320, 347-390
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/state/appStore.ts` around lines 162 - 178, The current scheduleProjectHydration flow fires multiple get().refreshLanes calls and only checks projectKey for staleness, so earlier slower responses (e.g., includeStatus: false) can overwrite newer richer laneSnapshots; add a monotonic per-project request token/version on the store (e.g., laneRefreshVersion or laneRefreshToken) that you increment before scheduling each refresh, pass that token into refreshLanes (or include it in the returned Promise context) and make the refresh response handler verify the token still matches before applying updates to laneSnapshots/projectKey; update the same pattern for the other refresh spots mentioned (ranges around lines 280-320 and 347-390) and ensure warmLaneStatusTimer/warmProviderModeTimer logic increments/attaches the token so out-of-order completions are ignored.
🟡 Minor comments (7)
apps/ios/ADE/Views/Lanes/LaneTypes.swift-132-140 (1)
132-140:⚠️ Potential issue | 🟡 MinorInclude
compareToinLaneDiffRequest.id.Two requests that differ only by
compareTocurrently share the same identity, so distinct diff requests can alias each other.🪪 Proposed fix
struct LaneDiffRequest: Identifiable { - var id: String { "\(laneId):\(mode):\(path ?? "none"):\(compareRef ?? "none")" } + var id: String { "\(laneId):\(mode):\(path ?? "none"):\(compareRef ?? "none"):\(compareTo ?? "none")" } let laneId: String let path: String? let mode: String let compareRef: String?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneTypes.swift` around lines 132 - 140, The computed id on LaneDiffRequest omits compareTo, causing distinct requests that only differ by compareTo to collide; update the id getter on the LaneDiffRequest struct to include compareTo (e.g., append compareTo with a default like "none") so the identifier uses laneId, mode, path, compareRef and compareTo to produce a unique id for each distinct request..factory/init.sh-6-8 (1)
6-8:⚠️ Potential issue | 🟡 MinorWrite the failure message to stderr.
This is an error path, so sending the message to stdout makes it harder for callers to separate normal output from failures.
🔧 Proposed fix
if ! command -v xcodebuild &> /dev/null; then - echo "ERROR: xcodebuild not found. Xcode must be installed." + echo "ERROR: xcodebuild not found. Xcode must be installed." >&2 exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.factory/init.sh around lines 6 - 8, The error message for the xcodebuild existence check is currently written to stdout; change the echo to write to stderr (e.g., use echo "ERROR: xcodebuild not found. Xcode must be installed." >&2 or printf to >&2) in the block that tests command -v xcodebuild so callers can distinguish error output from normal output; keep the existing exit 1 behavior unchanged.apps/ios/ADE/Views/Lanes/LaneChatSessionView.swift-85-99 (1)
85-99:⚠️ Potential issue | 🟡 MinorLabel the send button explicitly.
This is an icon-only control, so assistive tech falls back to the SF Symbol's generated label instead of the action the button performs. Please add an explicit accessibility label such as “Send message”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneChatSessionView.swift` around lines 85 - 99, The send button in LaneChatSessionView is an icon-only Button (using sending, composer, and sendMessage()) and lacks an explicit accessibility label; add a clear label like "Send message" by applying .accessibilityLabel("Send message") to the Button (or its Image) so assistive tech announces the action rather than the SF Symbol name, keeping the existing disabled logic (composer.trimmingCharacters... || sending) intact.apps/ios/ADE/Views/Lanes/LaneChatSessionView.swift-16-17 (1)
16-17:⚠️ Potential issue | 🟡 MinorThe retry instruction doesn't exist on this screen.
When transcript refresh fails after a send, the banner says “Pull to retry”, but this
ScrollViewhas no.refreshableand no explicit retry control. Either add a real retry affordance or change the copy to the actual next step.Also applies to: 149-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneChatSessionView.swift` around lines 16 - 17, The error is that the "Pull to retry" text is shown even though the ScrollView in LaneChatSessionView.swift (the ScrollView containing the VStack) has no .refreshable or other retry affordance; update the UI to either add a true retry control (e.g., attach .refreshable to the ScrollView and call the existing transcript refresh method) or change the banner copy to a correct action (e.g., "Tap to retry" and wire the banner/tap handler to call the same refresh function); locate references to ScrollView and the banner code near the VStack and the error-display logic (also check the similar text around the other occurrence) and implement one of these two fixes so the displayed instruction matches the available interaction.apps/ios/ADE/Views/Lanes/LaneAttachSheet.swift-59-60 (1)
59-60:⚠️ Potential issue | 🟡 MinorTrim the submitted name and path.
The button only trims for validation, but
attachLane(...)still receives the raw values. A pasted path with a trailing space/newline will fail against an otherwise valid worktree, and lane names can be stored with invisible whitespace.💡 Suggested fix
private func submit() async { do { busy = true errorMessage = nil - let lane = try await syncService.attachLane(name: name, attachedPath: attachedPath, description: description) + let lane = try await syncService.attachLane( + name: name.trimmingCharacters(in: .whitespacesAndNewlines), + attachedPath: attachedPath.trimmingCharacters(in: .whitespacesAndNewlines), + description: description.trimmingCharacters(in: .whitespacesAndNewlines) + ) await onComplete(lane.id) dismiss()Also applies to: 67-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneAttachSheet.swift` around lines 59 - 60, The submit uses trimmed validation but passes raw values to attachLane, so trim the inputs before calling attachLane and before saving to state: replace usages of name and attachedPath in the submit action(s) (the call to attachLane(...) and any storage assignment) with name.trimmingCharacters(in: .whitespacesAndNewlines) and attachedPath.trimmingCharacters(in: .whitespacesAndNewlines); ensure any other places noted (the block around lines 67-70) also use the trimmed values so stored lane names and paths never include incidental whitespace.apps/ios/ADE/Views/Lanes/LaneStackGraphSheet.swift-27-30 (1)
27-30:⚠️ Potential issue | 🟡 MinorKeep disconnected branches together.
remainingis flattened with a globalcreatedAtsort, so non-primary roots can get separated from their own descendants. For a stack graph, each disconnected root should still be emitted throughvisit(parentId:).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneStackGraphSheet.swift` around lines 27 - 30, The current code flattens all non-primary snapshots sorted by createdAt which can split disconnected branches; instead identify remaining roots (snapshots in remaining that have no parent inside remaining) by filtering remaining for snapshots where remaining.first(where: { $0.lane.id == snapshot.lane.parentId }) == nil, sort those roots by createdAt, then for each root emit its branch as [$0] + visit(parentId: $0.lane.id) and finally flatten those branches; replace the single sorted-remaining expression with this grouped-by-root logic using the existing primaryBranch, remaining, snapshots and visit(parentId:) symbols.apps/ios/ADE/Views/Lanes/LaneCreateSheet.swift-44-47 (1)
44-47:⚠️ Potential issue | 🟡 MinorRescue mode silently drops the description.
The form always collects
description, but the.rescueUnstagedbranch callscreateFromUnstaged(sourceLaneId:name:)and ignores it. Hide the field for this mode or thread the value through if it should be saved.Also applies to: 222-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneCreateSheet.swift` around lines 44 - 47, The form always collects `description` in LaneCreateSheet but the `.rescueUnstaged` path calls createFromUnstaged(sourceLaneId:name:) and drops it; either hide the LaneTextField for "Description" when `mode == .rescueUnstaged` or thread the value through by updating createFromUnstaged to accept a description parameter (e.g. createFromUnstaged(sourceLaneId:name:description:)) and update all callers (including the call sites referenced around lines 222-223) to pass `description`; locate the UI pieces as LaneTextField("Description", text: $description) and the logic in the rescueUnstaged branch and apply one of the two fixes consistently.
🧹 Nitpick comments (9)
apps/desktop/src/renderer/components/onboarding/DevToolsSection.tsx (1)
72-74: Consider removing the unusedtoolIdparameter.The
toolIdparameter is passed toToolCardbut is no longer used since the component now unconditionally uses Git styling. This dead parameter adds confusion about whether the component is designed to handle multiple tools.♻️ Proposed cleanup
-function ToolCard({ tool, platform, loading, toolId }: { tool: DevToolStatus | null; platform: NodeJS.Platform; loading: boolean; toolId: string }) { +function ToolCard({ tool, platform, loading }: { tool: DevToolStatus | null; platform: NodeJS.Platform; loading: boolean }) { const accentColor = COLORS.success; const Icon = GitBranch;And update the call site at line 58:
- <ToolCard tool={git} platform={platform} loading={loading && !result} toolId="git" /> + <ToolCard tool={git} platform={platform} loading={loading && !result} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/onboarding/DevToolsSection.tsx` around lines 72 - 74, Remove the dead toolId prop from the ToolCard component and all call sites: update the ToolCard function signature (function ToolCard({ tool, platform, loading }: { tool: DevToolStatus | null; platform: NodeJS.Platform; loading: boolean })) to drop toolId, and remove toolId from any JSX where ToolCard is instantiated; ensure any types or interfaces referencing toolId for ToolCard are updated as well and run type checks to confirm no remaining references.apps/desktop/src/renderer/browserMock.ts (1)
161-191: Return fresh snapshot objects from the mock to prevent shared-state leakage.
Right nowlistSnapshotsserves the same precomputed instances each call. Returning new objects per invocation makes tests and local UI behavior safer if any consumer mutates the payload.♻️ Suggested change
-function makeLaneSnapshot(lane: any): any { +function makeLaneSnapshot(lane: any): any { const runtimeBucket = lane.id === "lane-auth" || lane.id === "lane-checkout" ? "running" : lane.id === "lane-dashboard" || lane.id === "lane-api" ? "awaiting-input" : lane.id === "lane-perf" ? "ended" : "none"; const runtime = { bucket: runtimeBucket, runningCount: runtimeBucket === "running" ? 1 : 0, awaitingInputCount: runtimeBucket === "awaiting-input" ? 1 : 0, endedCount: runtimeBucket === "ended" ? 1 : 0, sessionCount: runtimeBucket === "none" ? 0 : 1, }; return { - lane, + lane: { ...lane }, runtime, @@ - listSnapshots: resolved(MOCK_LANES.map((lane) => makeLaneSnapshot(lane))), + listSnapshots: async () => MOCK_LANES.map((lane) => makeLaneSnapshot(lane)),Also applies to: 1130-1130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/browserMock.ts` around lines 161 - 191, The mock currently reuses precomputed snapshot objects which can cause shared-state leakage; update makeLaneSnapshot (and any related listSnapshots usage) to always construct and return fresh plain objects per call rather than returning or reusing previously created instances—ensure runtime, rebaseSuggestion, autoRebaseStatus, conflictStatus, stateSnapshot and the lane wrapper are newly created (or deep-cloned) each invocation so consumers mutating responses won't affect subsequent calls..factory/library/user-testing.md (1)
25-26: Optional: make the unit test count wording evergreen.
Hard-coded counts drift quickly; consider wording like “Current unit tests inADETests.swiftcover …” without a fixed number.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.factory/library/user-testing.md around lines 25 - 26, Replace the hard-coded test count line ("50 existing unit tests in ADETests.swift") with evergreen wording that doesn't include a specific number; for example update the line in .factory/library/user-testing.md to something like "Current unit tests in ADETests.swift cover …" or "Unit tests in ADETests.swift cover …" so the sentence remains accurate as tests change; edit the exact text where "ADETests.swift" is mentioned to remove the numeric literal..factory/services.yaml (1)
2-7: Consider normalizing all iOS commands to the same destination style.
build/test/build-for-testingnow use name+OS whiletypecheck/lintstill depend on a fixed simulator UUID. Standardizing these reduces machine-specific failures.♻️ Suggested change
commands: build: xcodebuild build -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro,OS=26.3.1' -quiet test: xcodebuild test -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro,OS=26.3.1' -quiet build-for-testing: xcodebuild build-for-testing -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro,OS=26.3.1' -quiet - typecheck: cd apps/ios && xcodebuild -project ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,id=2107A402-C2A7-4323-AF26-74A0AC406C44' -derivedDataPath /tmp/ade-build build - lint: cd apps/ios && xcodebuild -project ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,id=2107A402-C2A7-4323-AF26-74A0AC406C44' -derivedDataPath /tmp/ade-build analyze + typecheck: xcodebuild build -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro,OS=26.3.1' -quiet + lint: xcodebuild analyze -project apps/ios/ADE.xcodeproj -scheme ADE -destination 'platform=iOS Simulator,name=iPhone 17 Pro,OS=26.3.1' -quiet line-count: find apps/ios/ADE -name '*.swift' -exec wc -l {} + | sort -rn | head -20🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.factory/services.yaml around lines 2 - 7, The iOS CI commands in .factory/services.yaml are inconsistent: build/test/build-for-testing use destination 'platform=iOS Simulator,name=...,OS=...' while typecheck and lint use a fixed simulator UUID; update the typecheck and lint targets to use the same name+OS destination format (match the pattern used by build/test/build-for-testing) so all targets (build, test, build-for-testing, typecheck, lint) use a non-machine-specific simulator destination and avoid UUID-dependent failures.apps/ios/ADE/Views/Lanes/LaneFileTreeComponents.swift (1)
27-57: This eagerly renders the full diff tree.
VStackinsideScrollViewmaterializes every file row and expanded subtree up front. On large diffs that gets expensive quickly; switching the file/folder containers toLazyVStackkeeps this screen responsive without changing the UI behavior.Also applies to: 135-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneFileTreeComponents.swift` around lines 27 - 57, The current GlassSection content eagerly renders all file rows because it uses VStack, causing performance issues for large diffs; replace the eager VStack with LazyVStack (and any other VStack usages in the same file, e.g. the block around lines 135-198) so rows and expanded subtrees are materialized lazily; update the container that wraps the conditional bulk action, "No files." Text, and the LaneFileTreeNodeView (and any sibling VStack instances) to LazyVStack while preserving alignment and spacing parameters to maintain identical layout and behavior.apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift (2)
532-534: Consider makingcanRunLiveActionsprivate.This computed property appears to be internal to the view's logic. Marking it
privatewould improve encapsulation.♻️ Suggested change
- var canRunLiveActions: Bool { + private var canRunLiveActions: Bool { syncService.connectionState == .connected || syncService.connectionState == .syncing }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift` around lines 532 - 534, Make the computed property canRunLiveActions private to improve encapsulation: locate the var canRunLiveActions computed property (which checks syncService.connectionState == .connected || syncService.connectionState == .syncing) and change its access level to private so it isn’t exposed outside LaneDetailScreen's implementation.
14-27: Mark@Stateproperties asprivatewhere possible.SwiftLint correctly flags that SwiftUI
@Stateproperties should be private. This ensures state ownership is clear and prevents accidental external mutation.For properties accessed by extensions (like
detail,errorMessage,busyAction), consider usingprivate(set)or restructuring to keep state encapsulated.♻️ Proposed fix for state visibility
- `@State` var detail: LaneDetailPayload? - `@State` var errorMessage: String? - `@State` var busyAction: String? - `@State` var selectedDiffRequest: LaneDiffRequest? + `@State` private var detail: LaneDetailPayload? + `@State` private var errorMessage: String? + `@State` private var busyAction: String? + `@State` private var selectedDiffRequest: LaneDiffRequest? `@State` private var showStackGraph = false `@State` private var managePresented = false `@State` private var chatLaunchTarget: LaneChatLaunchTarget? `@State` private var lanePullRequests: [PullRequestListItem] = [] `@State` private var headerExpanded = true `@State` private var selectedSection: LaneDetailSection - `@State` var commitMessage = "" - `@State` var amendCommit = false - `@State` var stashMessage = "" - `@State` var confirmDiscardFile: FileChange? + `@State` private var commitMessage = "" + `@State` private var amendCommit = false + `@State` private var stashMessage = "" + `@State` private var confirmDiscardFile: FileChange? `@State` private var filesWorkspaceId: String?If these properties need to be accessed by extensions in separate files, consider moving the extension to this file or using a different state management approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift` around lines 14 - 27, Mark SwiftUI `@State` properties as private to enforce state ownership: change the declarations for showStackGraph, managePresented, chatLaunchTarget, lanePullRequests, headerExpanded, commitMessage, amendCommit, stashMessage, selectedDiffRequest, and confirmDiscardFile to use private (e.g., private `@State`) and make selectedSection private if it is not read from outside; for properties that are accessed by extensions (detail, errorMessage, busyAction) change them to private(set) `@State` so external readers can access but not mutate, or move the accessing extensions into this file if mutation is required. Ensure you update the property access modifiers consistently where these symbols (detail, errorMessage, busyAction, selectedDiffRequest, showStackGraph, managePresented, chatLaunchTarget, lanePullRequests, headerExpanded, selectedSection, commitMessage, amendCommit, stashMessage, confirmDiscardFile) are referenced.apps/ios/ADE/Views/Lanes/LaneHelpers.swift (1)
61-68: Sorting may produce inconsistent results whencreatedAtvalues are equal.When two lanes have the same
createdAttimestamp, the sort falls back to name comparison. However, if names are also equal (e.g., duplicate lane names), the sort order becomes non-deterministic. Consider adding the laneidas a final tiebreaker for stable sorting.♻️ Proposed fix for stable sorting
func laneListSortSnapshots(_ lhs: LaneListSnapshot, _ rhs: LaneListSnapshot) -> Bool { if lhs.lane.laneType == "primary" && rhs.lane.laneType != "primary" { return true } if lhs.lane.laneType != "primary" && rhs.lane.laneType == "primary" { return false } if lhs.lane.createdAt != rhs.lane.createdAt { return lhs.lane.createdAt > rhs.lane.createdAt } - return lhs.lane.name.localizedCaseInsensitiveCompare(rhs.lane.name) == .orderedAscending + let nameComparison = lhs.lane.name.localizedCaseInsensitiveCompare(rhs.lane.name) + if nameComparison != .orderedSame { + return nameComparison == .orderedAscending + } + return lhs.lane.id < rhs.lane.id }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneHelpers.swift` around lines 61 - 68, The comparator laneListSortSnapshots currently falls back to name comparison and can still be non-deterministic if createdAt and name are equal; update laneListSortSnapshots to add a final tiebreaker using a stable identifier (e.g., compare lhs.lane.id and rhs.lane.id) after the name comparison so equal timestamps and names produce a deterministic order; ensure the id comparison uses a consistent comparison (lexical or numeric depending on id type) and returns true/false accordingly.apps/ios/ADE/Views/Lanes/LaneListViewParts.swift (1)
433-438: Silent no-op when closing a pinned lane chip.
closeLaneChipsilently returns without feedback when attempting to close a pinned lane. This might be intentional UX (pinned lanes stay open), but consider whether the caller should be aware of this behavior or if it should be documented.If the behavior is intentional, a brief comment would improve clarity:
func closeLaneChip(_ laneId: String) { + // Pinned lanes cannot be closed from the chip; unpin first if pinnedLaneIds.contains(laneId) { return } openLaneIds.removeAll { $0 == laneId } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneListViewParts.swift` around lines 433 - 438, The function closeLaneChip currently silently returns when a laneId is in pinnedLaneIds, which makes the no-op behavior unclear to callers; either document this intent with a brief comment above closeLaneChip stating "pinned lanes cannot be closed" or change the API to make the behavior explicit (e.g., have closeLaneChip(_:) return Bool indicating success/failure or emit a log/callback) and update callers accordingly; reference the closeLaneChip function and the pinnedLaneIds and openLaneIds collections when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 755-765: buildLaneListSnapshots currently calls
sessionService.list({ limit: 500 }) which bypasses the runtime enrichment and
identity-chat filtering performed by IPC.sessionsList, causing badge/state
mismatches and missing awaiting-input lanes and truncating results; change
buildLaneListSnapshots to consume the already-enriched session view used by
IPC.sessionsList (or call the same enrichment helper that attaches
ptyService/agentChatService runtime state and applies identity chat filtering)
instead of calling sessionService.list directly, and remove the hard 500 limit
so full session counts are preserved (update callers of buildLaneListSnapshots
or its signature as needed to pass enrichedSessions).
- Around line 738-744: The current bucket selection logic sets bucket to
"running" whenever runningCount > 0, which masks lanes that have
awaitingInputCount > 0; change the condition order so awaitingInputCount is
checked before runningCount — e.g. set bucket to "awaiting-input" if
awaitingInputCount > 0, else "running" if runningCount > 0, else "ended" if
endedCount > 0, else "none"; update the ternary (or use an if/else) that assigns
bucket (the variable named bucket in this file) to reflect this new priority.
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 1555-1586: The current logic sets parentLaneId = null when
mainScore <= bestScore which is treated later as “no detection” and causes the
code to reapply explicitParentLaneId; instead, when deciding that the branch is
based on main (mainScore <= bestScore) do not unconditionally null out
parentLaneId — preserve any caller-supplied explicitParentLaneId: change the
block that sets parentLaneId = null so it only sets parentLaneId = null if
explicitParentLaneId is null/undefined, otherwise keep/assign
explicitParentLaneId (use the existing variables bestLaneId, mainScore,
bestScore, parentLaneId, explicitParentLaneId, importedHeadSha, defaultBaseRef
to locate the code).
In `@apps/desktop/src/main/services/memory/embeddingService.ts`:
- Around line 565-577: clearCache currently disposes and deletes the on-disk
install but leaves the in-memory cache and doesn't emit the model-missing
status, so subsequent embed() calls still hit old vectors before
ensureExtractor() runs; update clearCache to also clear the in-memory cache
(call cache.clear() or equivalent on the cache used by embed()) immediately
after dispose(), and ensure refreshCachedInstall() emits the "missing" status
(or explicitly emit the missing transition from clearCache) so listeners see the
state change; reference functions: clearCache, dispose, embed, ensureExtractor,
cache, and refreshCachedInstall.
- Around line 565-577: clearCache() currently calls dispose() which only
invalidates a pending load but doesn't await extractorPromise, causing a race
where files under installPath can be recreated; change clearCache() so that
after calling dispose() it awaits the existing extractorPromise (if any) to
settle (catching errors) before attempting fs.promises.rm and before calling
refreshCachedInstall(); reference the extractorPromise variable and ensure you
handle its possible undefined state and swallow or log errors from awaiting it
so removal runs only after the pipeline has finished.
In `@apps/ios/ADE/Services/Database.swift`:
- Around line 2240-2252: The code skips composite PK changes when only a single
primaryKeyColumn was recorded; fix by handling the case where pkColumns.count >
1 but only one PK value was recorded: if decodeCrsqlPkColumns(first.pk) fails,
try decodeCrsqlPk(first.pk) to get the single PK value, then locate which column
that value corresponds to (using primaryKeyColumn and pkColumns) and also try to
obtain any other PK column values from rowChanges (find changes whose column
matches each pkColumns[i]) before building pkPairs and calling fetchRow; also
update rowChangesRepresentDeletedRow usage to consider the full primary key (not
only the first PK column) so deletes for composite-key tables aren’t incorrectly
ignored. Use the symbols decodeCrsqlPk, decodeCrsqlPkColumns, first.pk,
primaryKeyColumn, pkColumns, rowChanges, pkPairs, fetchRow, and
rowChangesRepresentDeletedRow to locate and implement this logic.
In `@apps/ios/ADE/Views/Lanes/LaneBatchManageSheet.swift`:
- Around line 174-183: The loop deletes lanes in the incoming selection order
which can fail if a parent is processed before its child; reorder laneIds to
delete descendants before parents by sorting them into descendant-first order
prior to the loop (e.g. compute a depth for each laneId and sort by descending
depth or split laneId by "/" and sort by component count) so that when you
iterate and call syncService.deleteLane(laneId, deleteBranch: deleteMode !=
.worktree, deleteRemoteBranch: deleteMode == .remoteBranch, remoteName:
deleteRemoteName, force: deleteForce) you always delete children before parents
and then append to deletedLaneIds as before.
In `@apps/ios/ADE/Views/Lanes/LaneChatLaunchSheet.swift`:
- Around line 162-166: The Launch button currently allows submission when models
is empty (and selectedModelId may be empty or stale), so ensure
submit/createChatSession never runs with an invalid model: update the launch
enablement and submit path to require a non-empty, valid selectedModelId (e.g.,
change the Button .disabled rule to include selectedModelId.isEmpty OR
models.isEmpty && selectedModelId.isEmpty) and add a guard at the start of
submit() (or before calling createChatSession(...)) that validates
selectedModelId against the current models list (or returns/throws early) so
failed loadModels() calls cannot leave a stale/empty model to be used.
In `@apps/ios/ADE/Views/Lanes/LaneCreateSheet.swift`:
- Around line 204-237: After creating the lane (the result of
createLane/createChildLane/importBranch/createFromUnstaged stored in created),
immediately persist/complete the lane by calling onComplete(created.id) and
dismiss the sheet before running post-create setup
(initializeLaneEnvironment/applyLaneTemplate); then run env setup as a separate
try/catch so any QueuedRemoteCommandError or other errors coming from
initializeLaneEnvironment/applyLaneTemplate only set
envProgress/queuedNotice/errorMessage but do NOT leave the sheet open or allow
re-submission that would create a duplicate; ensure you still set envProgress
from the setup call but move onComplete(created.id) + dismiss() to directly
after creation and before the env setup calls.
In `@apps/ios/ADE/Views/Lanes/LaneDetailContentSections.swift`:
- Around line 33-35: The container view currently uses
.accessibilityElement(children: .combine) which collapses interactive children
(manage, expand, stack, PR buttons) into a single accessibility element; update
this to .accessibilityElement(children: .contain) in the same view chain (the
view that also uses .adeGlassCard and sets
.accessibilityLabel(headerAccessibilityLabel)) so child controls remain
individually focusable by VoiceOver while preserving the parent container
structure.
In `@apps/ios/ADE/Views/Lanes/LaneDiffScreen.swift`:
- Around line 99-104: The view currently relies solely on `diff` to show a
loading indicator, so if `request.path` is nil or `fetchFileDiff(...)` throws
the UI stays stuck on `ProgressView`; add a separate `@State private var
isLoading: Bool = false` and use it to drive the loading UI in the body (replace
the current `else { Spacer() ProgressView() ... }` branches), set `isLoading =
true` before starting the async task in the `onAppear`/task logic around
`fetchFileDiff(request:)`, and ensure you set `isLoading = false` in a `defer`
or in both success and catch paths while setting `diff = nil` when an error
occurs so the screen can show an empty/error state; apply the same pattern to
the second occurrence around lines 152-164 that also renders `ProgressView`.
- Around line 63-69: The Save action currently doesn't account for binary diffs
(diff.isBinary), so an unstaged binary diff can expose the Save button and cause
saveEditedFile(...) to call writeLaneFileText(...) with editedText, potentially
corrupting binaries; update the toolbar logic that decides whether to
show/enable Save (the same check using mode/path/side) to also check
diff.isBinary and hide/disable the Save button when diff.isBinary == true, and
add a defensive early return at the top of saveEditedFile(...) (or before
calling writeLaneFileText(...)) that aborts if the current diff.isBinary is
true; apply the same fixes to the other similar blocks referenced (the blocks
around the other occurrences mentioned).
In `@apps/ios/ADE/Views/Lanes/LaneListViewParts.swift`:
- Around line 214-226: The Menu containing the "Defer" and "Dismiss" actions
must respect the same canRunLiveActions flag as the "Rebase" Button; either
disable the entire Menu or add .disabled(!canRunLiveActions) to each menu item
that calls syncService.deferRebaseSuggestion() and
syncService.dismissRebaseSuggestion(), referencing the Menu declaration and the
two menu items ("Defer", "Dismiss") so the UI prevents invoking network actions
when canRunLiveActions is false.
In `@apps/ios/ADE/Views/Lanes/LaneManageSheet.swift`:
- Around line 40-49: The reparentCandidates computed property currently includes
every non-archived lane except the lane itself, allowing selecting descendants
as new parents; update reparentCandidates to exclude snapshot.lane's descendants
by computing the descendant IDs from allLaneSnapshots (e.g., traverse children
via the lane.parentId relationship or use an existing tree helper) and filter
out any lane whose id is equal to snapshot.lane.id or is contained in that
descendant ID set before sorting; ensure you reference and use allLaneSnapshots
and snapshot.lane.id when building the filter so no child/grandchild can appear
in the parent picker.
- Around line 101-116: The Save button is disabled whenever selectedParentLaneId
is empty, preventing clearing a parent; change the .disabled logic to only
disable when selectedParentLaneId equals the current parent (e.g. compare to
snapshot.lane.parentId or snapshot.lane.parentId ?? ""), and when calling
syncService.reparentLane pass nil for newParentLaneId if
selectedParentLaneId.isEmpty (e.g. newParentLaneId: selectedParentLaneId.isEmpty
? nil : selectedParentLaneId). Also ensure selectedParentLaneId is initialized
from snapshot.lane.parentId (use "" when nil) so the equality check works as
intended; keep the Picker root tag as "" to represent "no parent."
---
Outside diff comments:
In `@apps/desktop/src/renderer/state/appStore.ts`:
- Around line 162-178: The current scheduleProjectHydration flow fires multiple
get().refreshLanes calls and only checks projectKey for staleness, so earlier
slower responses (e.g., includeStatus: false) can overwrite newer richer
laneSnapshots; add a monotonic per-project request token/version on the store
(e.g., laneRefreshVersion or laneRefreshToken) that you increment before
scheduling each refresh, pass that token into refreshLanes (or include it in the
returned Promise context) and make the refresh response handler verify the token
still matches before applying updates to laneSnapshots/projectKey; update the
same pattern for the other refresh spots mentioned (ranges around lines 280-320
and 347-390) and ensure warmLaneStatusTimer/warmProviderModeTimer logic
increments/attaches the token so out-of-order completions are ignored.
---
Minor comments:
In @.factory/init.sh:
- Around line 6-8: The error message for the xcodebuild existence check is
currently written to stdout; change the echo to write to stderr (e.g., use echo
"ERROR: xcodebuild not found. Xcode must be installed." >&2 or printf to >&2) in
the block that tests command -v xcodebuild so callers can distinguish error
output from normal output; keep the existing exit 1 behavior unchanged.
In `@apps/ios/ADE/Views/Lanes/LaneAttachSheet.swift`:
- Around line 59-60: The submit uses trimmed validation but passes raw values to
attachLane, so trim the inputs before calling attachLane and before saving to
state: replace usages of name and attachedPath in the submit action(s) (the call
to attachLane(...) and any storage assignment) with name.trimmingCharacters(in:
.whitespacesAndNewlines) and attachedPath.trimmingCharacters(in:
.whitespacesAndNewlines); ensure any other places noted (the block around lines
67-70) also use the trimmed values so stored lane names and paths never include
incidental whitespace.
In `@apps/ios/ADE/Views/Lanes/LaneChatSessionView.swift`:
- Around line 85-99: The send button in LaneChatSessionView is an icon-only
Button (using sending, composer, and sendMessage()) and lacks an explicit
accessibility label; add a clear label like "Send message" by applying
.accessibilityLabel("Send message") to the Button (or its Image) so assistive
tech announces the action rather than the SF Symbol name, keeping the existing
disabled logic (composer.trimmingCharacters... || sending) intact.
- Around line 16-17: The error is that the "Pull to retry" text is shown even
though the ScrollView in LaneChatSessionView.swift (the ScrollView containing
the VStack) has no .refreshable or other retry affordance; update the UI to
either add a true retry control (e.g., attach .refreshable to the ScrollView and
call the existing transcript refresh method) or change the banner copy to a
correct action (e.g., "Tap to retry" and wire the banner/tap handler to call the
same refresh function); locate references to ScrollView and the banner code near
the VStack and the error-display logic (also check the similar text around the
other occurrence) and implement one of these two fixes so the displayed
instruction matches the available interaction.
In `@apps/ios/ADE/Views/Lanes/LaneCreateSheet.swift`:
- Around line 44-47: The form always collects `description` in LaneCreateSheet
but the `.rescueUnstaged` path calls createFromUnstaged(sourceLaneId:name:) and
drops it; either hide the LaneTextField for "Description" when `mode ==
.rescueUnstaged` or thread the value through by updating createFromUnstaged to
accept a description parameter (e.g.
createFromUnstaged(sourceLaneId:name:description:)) and update all callers
(including the call sites referenced around lines 222-223) to pass
`description`; locate the UI pieces as LaneTextField("Description", text:
$description) and the logic in the rescueUnstaged branch and apply one of the
two fixes consistently.
In `@apps/ios/ADE/Views/Lanes/LaneStackGraphSheet.swift`:
- Around line 27-30: The current code flattens all non-primary snapshots sorted
by createdAt which can split disconnected branches; instead identify remaining
roots (snapshots in remaining that have no parent inside remaining) by filtering
remaining for snapshots where remaining.first(where: { $0.lane.id ==
snapshot.lane.parentId }) == nil, sort those roots by createdAt, then for each
root emit its branch as [$0] + visit(parentId: $0.lane.id) and finally flatten
those branches; replace the single sorted-remaining expression with this
grouped-by-root logic using the existing primaryBranch, remaining, snapshots and
visit(parentId:) symbols.
In `@apps/ios/ADE/Views/Lanes/LaneTypes.swift`:
- Around line 132-140: The computed id on LaneDiffRequest omits compareTo,
causing distinct requests that only differ by compareTo to collide; update the
id getter on the LaneDiffRequest struct to include compareTo (e.g., append
compareTo with a default like "none") so the identifier uses laneId, mode, path,
compareRef and compareTo to produce a unique id for each distinct request.
---
Nitpick comments:
In @.factory/library/user-testing.md:
- Around line 25-26: Replace the hard-coded test count line ("50 existing unit
tests in ADETests.swift") with evergreen wording that doesn't include a specific
number; for example update the line in .factory/library/user-testing.md to
something like "Current unit tests in ADETests.swift cover …" or "Unit tests in
ADETests.swift cover …" so the sentence remains accurate as tests change; edit
the exact text where "ADETests.swift" is mentioned to remove the numeric
literal.
In @.factory/services.yaml:
- Around line 2-7: The iOS CI commands in .factory/services.yaml are
inconsistent: build/test/build-for-testing use destination 'platform=iOS
Simulator,name=...,OS=...' while typecheck and lint use a fixed simulator UUID;
update the typecheck and lint targets to use the same name+OS destination format
(match the pattern used by build/test/build-for-testing) so all targets (build,
test, build-for-testing, typecheck, lint) use a non-machine-specific simulator
destination and avoid UUID-dependent failures.
In `@apps/desktop/src/renderer/browserMock.ts`:
- Around line 161-191: The mock currently reuses precomputed snapshot objects
which can cause shared-state leakage; update makeLaneSnapshot (and any related
listSnapshots usage) to always construct and return fresh plain objects per call
rather than returning or reusing previously created instances—ensure runtime,
rebaseSuggestion, autoRebaseStatus, conflictStatus, stateSnapshot and the lane
wrapper are newly created (or deep-cloned) each invocation so consumers mutating
responses won't affect subsequent calls.
In `@apps/desktop/src/renderer/components/onboarding/DevToolsSection.tsx`:
- Around line 72-74: Remove the dead toolId prop from the ToolCard component and
all call sites: update the ToolCard function signature (function ToolCard({
tool, platform, loading }: { tool: DevToolStatus | null; platform:
NodeJS.Platform; loading: boolean })) to drop toolId, and remove toolId from any
JSX where ToolCard is instantiated; ensure any types or interfaces referencing
toolId for ToolCard are updated as well and run type checks to confirm no
remaining references.
In `@apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift`:
- Around line 532-534: Make the computed property canRunLiveActions private to
improve encapsulation: locate the var canRunLiveActions computed property (which
checks syncService.connectionState == .connected || syncService.connectionState
== .syncing) and change its access level to private so it isn’t exposed outside
LaneDetailScreen's implementation.
- Around line 14-27: Mark SwiftUI `@State` properties as private to enforce state
ownership: change the declarations for showStackGraph, managePresented,
chatLaunchTarget, lanePullRequests, headerExpanded, commitMessage, amendCommit,
stashMessage, selectedDiffRequest, and confirmDiscardFile to use private (e.g.,
private `@State`) and make selectedSection private if it is not read from outside;
for properties that are accessed by extensions (detail, errorMessage,
busyAction) change them to private(set) `@State` so external readers can access
but not mutate, or move the accessing extensions into this file if mutation is
required. Ensure you update the property access modifiers consistently where
these symbols (detail, errorMessage, busyAction, selectedDiffRequest,
showStackGraph, managePresented, chatLaunchTarget, lanePullRequests,
headerExpanded, selectedSection, commitMessage, amendCommit, stashMessage,
confirmDiscardFile) are referenced.
In `@apps/ios/ADE/Views/Lanes/LaneFileTreeComponents.swift`:
- Around line 27-57: The current GlassSection content eagerly renders all file
rows because it uses VStack, causing performance issues for large diffs; replace
the eager VStack with LazyVStack (and any other VStack usages in the same file,
e.g. the block around lines 135-198) so rows and expanded subtrees are
materialized lazily; update the container that wraps the conditional bulk
action, "No files." Text, and the LaneFileTreeNodeView (and any sibling VStack
instances) to LazyVStack while preserving alignment and spacing parameters to
maintain identical layout and behavior.
In `@apps/ios/ADE/Views/Lanes/LaneHelpers.swift`:
- Around line 61-68: The comparator laneListSortSnapshots currently falls back
to name comparison and can still be non-deterministic if createdAt and name are
equal; update laneListSortSnapshots to add a final tiebreaker using a stable
identifier (e.g., compare lhs.lane.id and rhs.lane.id) after the name comparison
so equal timestamps and names produce a deterministic order; ensure the id
comparison uses a consistent comparison (lexical or numeric depending on id
type) and returns true/false accordingly.
In `@apps/ios/ADE/Views/Lanes/LaneListViewParts.swift`:
- Around line 433-438: The function closeLaneChip currently silently returns
when a laneId is in pinnedLaneIds, which makes the no-op behavior unclear to
callers; either document this intent with a brief comment above closeLaneChip
stating "pinned lanes cannot be closed" or change the API to make the behavior
explicit (e.g., have closeLaneChip(_:) return Bool indicating success/failure or
emit a log/callback) and update callers accordingly; reference the closeLaneChip
function and the pinnedLaneIds and openLaneIds collections when applying the
change.
🪄 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: aa4aaab0-c45c-461d-bb2c-1a9f6ffa4246
⛔ Files ignored due to path filters (6)
apps/ios/ADE.xcodeproj/project.pbxprojis excluded by!**/*.xcodeproj/project.pbxprojapps/web/public/app-icon.pngis excluded by!**/*.pngapps/web/public/images/splash/left.pngis excluded by!**/*.pngapps/web/public/images/splash/middle.pngis excluded by!**/*.pngapps/web/public/images/splash/right.pngis excluded by!**/*.pngapps/web/public/logo.pngis excluded by!**/*.png
📒 Files selected for processing (46)
.factory/init.sh.factory/library/architecture.md.factory/library/environment.md.factory/library/user-testing.md.factory/services.yaml.factory/skills/ios-worker/SKILL.mdapps/desktop/src/main/services/devTools/devToolsService.test.tsapps/desktop/src/main/services/devTools/devToolsService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/memory/embeddingService.test.tsapps/desktop/src/main/services/memory/embeddingService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/onboarding/DevToolsSection.test.tsxapps/desktop/src/renderer/components/onboarding/DevToolsSection.tsxapps/desktop/src/renderer/components/onboarding/EmbeddingsSection.tsxapps/desktop/src/renderer/components/settings/MemoryHealthTab.tsxapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/devTools.tsapps/ios/ADE/Services/Database.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Lanes/LaneAttachSheet.swiftapps/ios/ADE/Views/Lanes/LaneBatchManageSheet.swiftapps/ios/ADE/Views/Lanes/LaneChatLaunchSheet.swiftapps/ios/ADE/Views/Lanes/LaneChatSessionView.swiftapps/ios/ADE/Views/Lanes/LaneComponents.swiftapps/ios/ADE/Views/Lanes/LaneCreateSheet.swiftapps/ios/ADE/Views/Lanes/LaneDetailContentSections.swiftapps/ios/ADE/Views/Lanes/LaneDetailGitSection.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/LaneSessionTranscriptView.swiftapps/ios/ADE/Views/Lanes/LaneStackGraphSheet.swiftapps/ios/ADE/Views/Lanes/LaneTypes.swiftapps/ios/ADE/Views/LanesTabView.swiftapps/ios/ADE/Views/PRsTabView.swiftapps/ios/ADETests/ADETests.swift
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/components/onboarding/DevToolsSection.test.tsx
Comment on lines
+99
to
+104
| } else { | ||
| Spacer() | ||
| ProgressView() | ||
| .tint(ADEColor.accent) | ||
| Spacer() | ||
| } |
There was a problem hiding this comment.
Exit loading state when no diff can be shown.
If request.path is nil, or fetchFileDiff(...) throws, diff never becomes non-nil and the body keeps rendering ProgressView forever beneath the error banner. Track loading separately so the screen can settle into an empty/error state once the task completes.
💡 Suggested fix
struct LaneDiffScreen: View {
`@Environment`(\.dismiss) private var dismiss
`@EnvironmentObject` private var syncService: SyncService
let request: LaneDiffRequest
`@State` private var diff: FileDiff?
+ `@State` private var isLoading = true
`@State` private var editedText = ""
`@State` private var errorMessage: String?
`@State` private var isSaving = false
`@State` private var side = "modified"
@@
- } else {
+ } else if isLoading {
Spacer()
ProgressView()
.tint(ADEColor.accent)
Spacer()
+ } else {
+ GlassSection(title: "Diff unavailable") {
+ Text(errorMessage ?? "File path unavailable.")
+ .font(.subheadline)
+ .foregroundStyle(ADEColor.textSecondary)
+ }
+ .padding(.horizontal, 16)
}
@@
.task {
+ isLoading = true
+ defer { isLoading = false }
do {
try await load()
} catch {
errorMessage = error.localizedDescription
}
}Also applies to: 152-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Views/Lanes/LaneDiffScreen.swift` around lines 99 - 104, The
view currently relies solely on `diff` to show a loading indicator, so if
`request.path` is nil or `fetchFileDiff(...)` throws the UI stays stuck on
`ProgressView`; add a separate `@State private var isLoading: Bool = false` and
use it to drive the loading UI in the body (replace the current `else { Spacer()
ProgressView() ... }` branches), set `isLoading = true` before starting the
async task in the `onAppear`/task logic around `fetchFileDiff(request:)`, and
ensure you set `isLoading = false` in a `defer` or in both success and catch
paths while setting `diff = nil` when an error occurs so the screen can show an
empty/error state; apply the same pattern to the second occurrence around lines
152-164 that also renders `ProgressView`.
Fix bucket priority (awaiting-input before running), enrich lane snapshots with runtime state, isolate auxiliary Promise.all failures, add monotonic refresh tokens, replace console.warn with structured logger, and remove dead setLaneSnapshots. On iOS: fix composite PK replication, descendant-first delete ordering, model validation, dismiss-before-env-setup, accessibility improvements, binary diff save protection, lazy file tree rendering, and deterministic sort tiebreakers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…anges Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/components/lanes/LanesPage.tsx (1)
262-279:⚠️ Potential issue | 🟠 MajorSurface awaiting-input lanes before running lanes.
The current
bucketRankstill gives"running"a higher priority than"awaiting-input", so lanes blocked on user input sink below actively running lanes in the default tab order. Swap those two ranks so needs-attention lanes stay at the top.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx` around lines 262 - 279, The bucket ranking in LanesPage.tsx currently prioritizes "running" over "awaiting-input", causing lanes that need user input to sort below active lanes; update the bucketRank mapping (the const bucketRank used by the sort of base from laneFilterMatchedLanes and laneRuntimeById) so "awaiting-input" has a lower numeric rank than "running" (i.e., swap their rank values) to surface awaiting-input lanes before running ones in the default sort.
♻️ Duplicate comments (2)
apps/ios/ADE/Views/Lanes/LaneChatLaunchSheet.swift (1)
162-170:⚠️ Potential issue | 🟠 MajorReset model state immediately on provider changes.
The launch path still trusts
modelsandselectedModelId, but those values remain from the previous provider untillistChatModels(...)finishes. A slow or failed Claude→Codex switch can therefore still submit a Claude model, and a late response can overwrite the newer provider's list. Clear the selection up front and ignore results that no longer match the currentprovider.Also applies to: 176-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneChatLaunchSheet.swift` around lines 162 - 170, When provider changes we must immediately clear model state and ignore stale async results: call loadModels(resetSelection: true) (or explicitly set models = [] and selectedModelId = "" right away) in the .task(id: provider) block, and inside the async path that awaits listChatModels(...) capture the current provider into a local constant (e.g., let requestedProvider = provider) before awaiting and after the await verify that provider == requestedProvider before assigning to models or selectedModelId; also ensure submit() checks selectedModelId is for the current provider (or disable Launch until the provider and models are in sync). Use the loadModels(resetSelection:) and listChatModels(...) call sites and the models/selectedModelId state to locate where to clear state and add the provider-check guard to ignore late responses.apps/ios/ADE/Services/Database.swift (1)
2240-2269:⚠️ Potential issue | 🔴 CriticalComposite-key replication is still not row-unique.
This recovery path still assumes
change.pkuniquely identifies one row. Our local CRR triggers still write onlyprimaryKeyColumnintopk, so delete-only groups can’t recover the remaining PK columns here, and rows that share the first PK component can still be merged into one group before recovery runs. The fix needs to start at the trigger layer by emitting the packed full PK for insert/update/delete and then grouping/applying by that full value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Services/Database.swift` around lines 2240 - 2269, The current recovery in Database.swift (in the block using decodeCrsqlPk, decodeCrsqlPkColumns, primaryKeyColumn, rowChanges and tableInfo.primaryKeyColumns) is unsafe because it assumes change.pk can be used to uniquely identify a row for composite keys; instead update the trigger layer to always emit the packed full primary key for all operations (insert/update/delete) so change.pk contains the complete composite-key encoding, then simplify the Database.swift logic to skip the complex partial-recovery branch and always decode the full PK via decodeCrsqlPk or decodeCrsqlPkColumns (or bail if decoding fails). Concretely: modify the DB sync triggers to write the full packed PK into change.pk, and in the code paths that read first.pk (where decodeCrsqlPk / decodeCrsqlPkColumns are used) remove the fallback that maps single PK values into primaryKeyColumn using rowChanges and require a full PK or skip the change.
🧹 Nitpick comments (6)
apps/desktop/src/renderer/components/onboarding/DevToolsSection.tsx (1)
87-87: Derive requirement copy fromtool.requiredto avoid drift
statusColoralready depends ontool.required, but the text is hardcoded as required. Derive the label from the same boolean so status and copy can’t diverge.♻️ Proposed change
- const requirementLabel = "Required to continue setup."; + const requirementLabel = tool.required + ? "Required to continue setup." + : "Optional for setup.";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/onboarding/DevToolsSection.tsx` at line 87, The hardcoded requirementLabel ("Required to continue setup.") can drift from the boolean used for status; change requirementLabel to derive its text from the same source boolean (tool.required) so statusColor and the copy stay consistent—e.g., compute requirementLabel using tool.required (and optionally invert or choose an alternate message when false) and use that variable where currently the hardcoded string is used; update any related UI text to reference requirementLabel and ensure statusColor still reads tool.required for consistency.apps/desktop/src/main/services/ipc/registerIpc.ts (1)
755-786: Reuse this helper fromIPC.sessionsListtoo.The same identity-session filtering and chat
runtimeStatemapping still exist again later in this file around Lines 3855-3883. MovingIPC.sessionsListonto this helper, or a shared variant, will keep the lane snapshot and session list surfaces from drifting the next time that mapping changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ipc/registerIpc.ts` around lines 755 - 786, The session identity-filtering and chat runtimeState mapping duplicated in IPC.sessionsList should be centralized: extract or reuse the existing enrichSessionsForLaneList helper by having IPC.sessionsList call enrichSessionsForLaneList (passing sessionService, ptyService, agentChatService) instead of duplicating logic; ensure the helper retains the same behavior (use agentChatService.listSessions with includeIdentity, filter out sessions with chat.identityKey, map chat.awaitingInput -> runtimeState "waiting-input", chat.status "active" -> "running", "idle" -> "idle", and leave non-chat or non-running sessions unchanged) so both the lane snapshot and IPC.sessionsList use identical session enrichment.apps/ios/ADE/Views/Lanes/LaneHelpers.swift (1)
254-261: Consider simplifying the fallback formatter allocation.Line 257 creates a new
ISO8601DateFormatter()on every call when the cached formatter fails. Since the cached formatter already haswithFractionalSeconds, the fallback without it handles timestamps lacking fractional seconds. Consider caching both formatters to avoid allocation on every fallback:♻️ Suggested refactor
+private let cachedISO8601FormatterNoFractional: ISO8601DateFormatter = { + let f = ISO8601DateFormatter() + f.formatOptions = [.withInternetDateTime] + return f +}() + func relativeTimestamp(_ timestamp: String?) -> String { guard let timestamp else { return "Unknown" } - guard let date = cachedISO8601Formatter.date(from: timestamp) - ?? ISO8601DateFormatter().date(from: timestamp) else { + guard let date = cachedISO8601Formatter.date(from: timestamp) + ?? cachedISO8601FormatterNoFractional.date(from: timestamp) else { return "Unknown" } return cachedRelativeDateFormatter.localizedString(for: date, relativeTo: Date()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneHelpers.swift` around lines 254 - 261, The relativeTimestamp(_:) function currently allocates a new ISO8601DateFormatter() on every fallback; instead create and reuse a second cached formatter (e.g., cachedISO8601FormatterNoFractional or similar) for timestamps without fractional seconds and try that before giving up. Update the parsing logic in relativeTimestamp to attempt date = cachedISO8601Formatter.date(from: timestamp) ?? cachedISO8601FormatterNoFractional.date(from: timestamp) and keep returning cachedRelativeDateFormatter.localizedString(for: date, relativeTo: Date()) on success, ensuring both formatter instances are created once (lazy/static) and reused.apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift (2)
14-17: Consider making@Stateproperties private per SwiftLint recommendation.SwiftLint flags these
@Stateproperties as non-private. While some likedetailandbusyActionuseprivate(set)to allow external reading, others likeerrorMessage,selectedDiffRequest,commitMessage,amendCommit,stashMessage, andconfirmDiscardFileappear to be intentionally non-private for external access (possibly for testing or parent view coordination).If these need external access, consider using
@Bindingpassed from parent views instead. If they're only used internally, addingprivatewould align with SwiftUI best practices.Also applies to: 24-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift` around lines 14 - 17, The `@State` properties in LaneDetailScreen (detail, errorMessage, busyAction, selectedDiffRequest, commitMessage, amendCommit, stashMessage, confirmDiscardFile) should be made private unless they must be read or mutated by parent views; change internal-only state to private (e.g., private `@State` var errorMessage: String?) and keep private(set) for properties that should be read externally (detail, busyAction) or convert externally-mutated state to `@Binding` on LaneDetailScreen’s initializer if the parent needs write access; update usages within LaneDetailScreen to the new access levels and adjust any parent callers to pass bindings for properties that were made `@Binding`.
644-659: Clarify intent of silently ignoringfetchGiterrors.The
try?on line 646 silently ignores any errors fromsyncService.fetchGit(laneId:). If this is intentional (e.g., fetch is best-effort before checking sync status), consider adding a brief comment explaining why the error is acceptable here. If errors should propagate, remove the?.func runRebaseAndPush() async throws { try await syncService.startLaneRebase(laneId: laneId, scope: "lane_only", pushMode: "none") - try? await syncService.fetchGit(laneId: laneId) + // Best-effort fetch to update remote refs before checking sync status + try? await syncService.fetchGit(laneId: laneId) let syncStatus = try await syncService.fetchSyncStatus(laneId: laneId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift` around lines 644 - 659, The silent ignore of errors from syncService.fetchGit(laneId:) inside runRebaseAndPush is unclear; either remove the optional try so errors propagate (change try? to try) or, if fetch is truly best-effort, keep try? but add a concise comment above that call explaining why fetch failures are acceptable (e.g., "best-effort preflight; continue to rely on server-side syncStatus"). Update the runRebaseAndPush implementation accordingly referencing syncService.fetchGit(laneId:) and runRebaseAndPush to make the intent explicit.apps/ios/ADE/Views/Lanes/LaneListViewParts.swift (1)
58-67: Task closure within notice action could benefit from explicit capture list.The
Taskblock in theactionclosure capturessyncServiceandself(viareload). WhilesyncServiceis an@EnvironmentObject(reference type) andreloadis a method on the extension, this pattern is generally safe in SwiftUI. However, for clarity and to prevent potential retain cycles in edge cases, consider using[weak syncService]or restructuring if the view could be dismissed while the task is running.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneListViewParts.swift` around lines 58 - 67, The Task started in the action closure currently captures syncService and self strongly; change it to use an explicit capture list like [weak syncService, weak self] and safely unwrap them inside the Task before calling reconnectIfPossible() and reload(refreshRemote:). Specifically, wrap the Task closure with the weak capture list, then guard-let or use optional chaining to call await syncService.reconnectIfPossible() and await self.reload(refreshRemote: true) only if the weak references remain, leaving the existing settingsPresented logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/main.ts`:
- Line 2298: The global env override ADE_MCP_SOCKET_PATH causes all
projectContexts to share one mcpSocketPath; change the mcpSocketPath assignment
in main.ts so the override is either scoped to the active project context or
namespaced per project (e.g. read process.env using a project identifier like
ADE_MCP_SOCKET_PATH_{projectId} or only apply process.env.ADE_MCP_SOCKET_PATH
when the context being created/activated is the single active context). Update
the symbol where mcpSocketPath is computed (the current const mcpSocketPath =
process.env.ADE_MCP_SOCKET_PATH?.trim() || adePaths.socketPath) to use a
project-scoped env key or conditional logic based on the active project in
projectContexts so each project gets its own socket path and avoids
EADDRINUSE/orphaned sockets.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 792-798: The Promise.resolve(...) wrappers won't catch synchronous
throws from service methods; update each element in the Promise.all array to
call the service inside an async IIFE with try/catch so both sync and async
errors are handled. For example, replace
Promise.resolve(args.rebaseSuggestionService?.listSuggestions() ?? []).catch(()
=> []) with (async () => { try { return await
args.rebaseSuggestionService?.listSuggestions() ?? []; } catch { return []; }
})(), and do the same pattern for autoRebaseService.listStatuses(),
laneService.listStateSnapshots(), and conflictService.getBatchAssessment(); keep
enrichSessionsForLaneList(...) as-is.
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 1561-1582: The code in laneService.ts uses runGit(["rev-parse",
defaultBaseRef]) which resolves the local branch tip and can be stale; change
the logic that computes mainSha (the runGit call and its surrounding block where
mainShaRes, mbRes, d1, d2, and mainScore are computed) to prefer a remote-aware
ref (e.g., "origin/<defaultBaseRef>" or "refs/remotes/origin/<defaultBaseRef>")
when resolving the default base; implement a fallback strategy: first try
rev-parse on the remote-aware ref, then fall back to the local defaultBaseRef if
the remote ref is missing, and keep the existing merge-base and rev-list scoring
logic unchanged once a valid mainSha is obtained.
In `@apps/desktop/src/main/services/state/kvDb.ts`:
- Around line 69-75: The code creates a DatabaseSync directly in
recentProjectSummary (via new DatabaseSync(...)) which bypasses the busy_timeout
set in openRawDatabase; update recentProjectSummary to either call
openRawDatabase(dbPath) instead of new DatabaseSync(...) or, if keeping direct
instantiation, call db.exec("PRAGMA busy_timeout = 5000") on that DatabaseSync
instance immediately after creation so it uses the same lock-wait behavior as
openRawDatabase.
In `@apps/desktop/src/renderer/state/appStore.ts`:
- Around line 281-291: The refreshLanes implementation can race with
project-switch IPC calls because laneRefreshVersion is incremented after
awaiting window IPCs; increment/invalidate laneRefreshVersion before any awaited
project switch so in-flight refreshes are considered stale immediately.
Specifically, in refreshLanes (and the similar blocks around the other
occurrences noted), bump the global laneRefreshVersion token (e.g.,
++laneRefreshVersion) before calling or awaiting window.ade.openRepo /
window.ade.switchProjectToPath / any IPC that may change getProject(), then
proceed to await window.ade.lanes.listSnapshots and compare tokens as before;
this ensures stale snapshots from a concurrent refresh won’t overwrite the new
project’s state.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 958-964: reparentLane currently omits the "newParentLaneId" key
when newParentLaneId is nil/empty which triggers server-side validation
(requireString); always include the "newParentLaneId" key in the args dictionary
before calling sendCommand — if newParentLaneId is nil or empty, set
args["newParentLaneId"] to a non-empty sentinel value (e.g., "ROOT" or another
agreed marker) so the server receives a defined non-empty string; update the
reparentLane function (the args variable and the sendCommand call) accordingly.
In `@apps/ios/ADE/Views/Lanes/LaneChatLaunchSheet.swift`:
- Around line 75-79: selectedReasoningEffort is not clamped to the currently
selected model, so changing selectedModelId can leave a stale value and submit()
may forward an invalid effort; update the code to reset or revalidate
selectedReasoningEffort inside the selectedModelId didSet / onChange handler (or
equivalent SwiftUI binding) so that when selectedModelId changes you set
selectedReasoningEffort = nil or to a default that exists in
selectedModel?.reasoningEfforts, and modify submit() to only include
selectedReasoningEffort if selectedModel?.reasoningEfforts contains it (check
against selectedModel?.reasoningEfforts), applying the same validation logic
wherever reasoning effort is read (e.g., the UI sections around lines 106-124
and 195-207).
In `@apps/ios/ADE/Views/Lanes/LaneChatSessionView.swift`:
- Around line 86-92: The send button allows racey duplicate sends because the
Task closure reads composer later; fix by preventing re-entrancy: capture the
current composer text into a local constant and atomically set the sending flag
(or clear composer) on the main actor before creating the Task, then call
sendMessage(using that captured text) from the Task; update
sendMessage/sendMessage() usage as needed (or add a sendMessage(text:) overload)
to ensure the async Task cannot re-read a changed composer and cannot be queued
while sending is true. Reference symbols: Button action, composer, sending,
sendMessage().
- Around line 127-130: Concurrent transcript fetches can race and overwrite
newer state; guard the assignments to transcript and errorMessage in the async
refresh paths by using a monotonic request token or canceling superseded Tasks
so only the latest fetchChatTranscript(sessionId:) result updates the view.
Implement a shared requestId (or keep a reference to the active Task) checked
after await and before setting transcript and errorMessage (both in the block
that assigns transcript = try await syncService.fetchChatTranscript(sessionId:
summary.sessionId) / errorMessage = nil and the similar block at the other
refresh path) to ignore older results or cancel prior loads.
In `@apps/ios/ADE/Views/Lanes/LaneDiffScreen.swift`:
- Around line 115-127: The Save button and its action are currently allowed when
diff is nil because diff?.isBinary != true is true for nil; update the guards so
both the ToolbarItem/Button rendering and the saveEditedFile call require a
non-nil, non-binary diff (e.g. check diff != nil && diff?.isBinary == false or
unwrap diff once) and only enable the button when that condition and !isSaving
hold; apply the same fix to the other occurrence that uses the same logic (the
block around saveEditedFile at the later location) so saveEditedFile cannot run
with an unloaded diff and overwrite editedText.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx`:
- Around line 262-279: The bucket ranking in LanesPage.tsx currently prioritizes
"running" over "awaiting-input", causing lanes that need user input to sort
below active lanes; update the bucketRank mapping (the const bucketRank used by
the sort of base from laneFilterMatchedLanes and laneRuntimeById) so
"awaiting-input" has a lower numeric rank than "running" (i.e., swap their rank
values) to surface awaiting-input lanes before running ones in the default sort.
---
Duplicate comments:
In `@apps/ios/ADE/Services/Database.swift`:
- Around line 2240-2269: The current recovery in Database.swift (in the block
using decodeCrsqlPk, decodeCrsqlPkColumns, primaryKeyColumn, rowChanges and
tableInfo.primaryKeyColumns) is unsafe because it assumes change.pk can be used
to uniquely identify a row for composite keys; instead update the trigger layer
to always emit the packed full primary key for all operations
(insert/update/delete) so change.pk contains the complete composite-key
encoding, then simplify the Database.swift logic to skip the complex
partial-recovery branch and always decode the full PK via decodeCrsqlPk or
decodeCrsqlPkColumns (or bail if decoding fails). Concretely: modify the DB sync
triggers to write the full packed PK into change.pk, and in the code paths that
read first.pk (where decodeCrsqlPk / decodeCrsqlPkColumns are used) remove the
fallback that maps single PK values into primaryKeyColumn using rowChanges and
require a full PK or skip the change.
In `@apps/ios/ADE/Views/Lanes/LaneChatLaunchSheet.swift`:
- Around line 162-170: When provider changes we must immediately clear model
state and ignore stale async results: call loadModels(resetSelection: true) (or
explicitly set models = [] and selectedModelId = "" right away) in the .task(id:
provider) block, and inside the async path that awaits listChatModels(...)
capture the current provider into a local constant (e.g., let requestedProvider
= provider) before awaiting and after the await verify that provider ==
requestedProvider before assigning to models or selectedModelId; also ensure
submit() checks selectedModelId is for the current provider (or disable Launch
until the provider and models are in sync). Use the loadModels(resetSelection:)
and listChatModels(...) call sites and the models/selectedModelId state to
locate where to clear state and add the provider-check guard to ignore late
responses.
---
Nitpick comments:
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 755-786: The session identity-filtering and chat runtimeState
mapping duplicated in IPC.sessionsList should be centralized: extract or reuse
the existing enrichSessionsForLaneList helper by having IPC.sessionsList call
enrichSessionsForLaneList (passing sessionService, ptyService, agentChatService)
instead of duplicating logic; ensure the helper retains the same behavior (use
agentChatService.listSessions with includeIdentity, filter out sessions with
chat.identityKey, map chat.awaitingInput -> runtimeState "waiting-input",
chat.status "active" -> "running", "idle" -> "idle", and leave non-chat or
non-running sessions unchanged) so both the lane snapshot and IPC.sessionsList
use identical session enrichment.
In `@apps/desktop/src/renderer/components/onboarding/DevToolsSection.tsx`:
- Line 87: The hardcoded requirementLabel ("Required to continue setup.") can
drift from the boolean used for status; change requirementLabel to derive its
text from the same source boolean (tool.required) so statusColor and the copy
stay consistent—e.g., compute requirementLabel using tool.required (and
optionally invert or choose an alternate message when false) and use that
variable where currently the hardcoded string is used; update any related UI
text to reference requirementLabel and ensure statusColor still reads
tool.required for consistency.
In `@apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift`:
- Around line 14-17: The `@State` properties in LaneDetailScreen (detail,
errorMessage, busyAction, selectedDiffRequest, commitMessage, amendCommit,
stashMessage, confirmDiscardFile) should be made private unless they must be
read or mutated by parent views; change internal-only state to private (e.g.,
private `@State` var errorMessage: String?) and keep private(set) for properties
that should be read externally (detail, busyAction) or convert
externally-mutated state to `@Binding` on LaneDetailScreen’s initializer if the
parent needs write access; update usages within LaneDetailScreen to the new
access levels and adjust any parent callers to pass bindings for properties that
were made `@Binding`.
- Around line 644-659: The silent ignore of errors from
syncService.fetchGit(laneId:) inside runRebaseAndPush is unclear; either remove
the optional try so errors propagate (change try? to try) or, if fetch is truly
best-effort, keep try? but add a concise comment above that call explaining why
fetch failures are acceptable (e.g., "best-effort preflight; continue to rely on
server-side syncStatus"). Update the runRebaseAndPush implementation accordingly
referencing syncService.fetchGit(laneId:) and runRebaseAndPush to make the
intent explicit.
In `@apps/ios/ADE/Views/Lanes/LaneHelpers.swift`:
- Around line 254-261: The relativeTimestamp(_:) function currently allocates a
new ISO8601DateFormatter() on every fallback; instead create and reuse a second
cached formatter (e.g., cachedISO8601FormatterNoFractional or similar) for
timestamps without fractional seconds and try that before giving up. Update the
parsing logic in relativeTimestamp to attempt date =
cachedISO8601Formatter.date(from: timestamp) ??
cachedISO8601FormatterNoFractional.date(from: timestamp) and keep returning
cachedRelativeDateFormatter.localizedString(for: date, relativeTo: Date()) on
success, ensuring both formatter instances are created once (lazy/static) and
reused.
In `@apps/ios/ADE/Views/Lanes/LaneListViewParts.swift`:
- Around line 58-67: The Task started in the action closure currently captures
syncService and self strongly; change it to use an explicit capture list like
[weak syncService, weak self] and safely unwrap them inside the Task before
calling reconnectIfPossible() and reload(refreshRemote:). Specifically, wrap the
Task closure with the weak capture list, then guard-let or use optional chaining
to call await syncService.reconnectIfPossible() and await
self.reload(refreshRemote: true) only if the weak references remain, leaving the
existing settingsPresented logic unchanged.
🪄 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: 8f446104-38a3-434f-a152-164486a68c24
📒 Files selected for processing (28)
.factory/init.sh.factory/library/user-testing.md.factory/services.yamlapps/desktop/src/main/main.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/memory/embeddingService.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/onboarding/DevToolsSection.tsxapps/desktop/src/renderer/state/appStore.tsapps/ios/ADE/Services/Database.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Lanes/LaneAttachSheet.swiftapps/ios/ADE/Views/Lanes/LaneBatchManageSheet.swiftapps/ios/ADE/Views/Lanes/LaneChatLaunchSheet.swiftapps/ios/ADE/Views/Lanes/LaneChatSessionView.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/LaneStackGraphSheet.swiftapps/ios/ADE/Views/Lanes/LaneTypes.swift
✅ Files skipped from review due to trivial changes (4)
- apps/ios/ADE/Views/Lanes/LaneBatchManageSheet.swift
- .factory/library/user-testing.md
- apps/ios/ADE/Views/Lanes/LaneCreateSheet.swift
- apps/ios/ADE/Views/Lanes/LaneFileTreeComponents.swift
🚧 Files skipped from review as they are similar to previous changes (6)
- .factory/init.sh
- apps/desktop/src/main/services/memory/embeddingService.ts
- apps/desktop/src/renderer/browserMock.ts
- apps/ios/ADE/Views/Lanes/LaneTypes.swift
- apps/ios/ADE/Views/Lanes/LaneManageSheet.swift
- .factory/services.yaml
Comment on lines
+792
to
+798
| const [sessions, rebaseSuggestions, autoRebaseStatuses, stateSnapshots, batchAssessment] = await Promise.all([ | ||
| enrichSessionsForLaneList(args), | ||
| Promise.resolve(args.rebaseSuggestionService?.listSuggestions() ?? []).catch(() => []), | ||
| Promise.resolve(args.autoRebaseService?.listStatuses() ?? []).catch(() => []), | ||
| Promise.resolve(args.laneService.listStateSnapshots()).catch(() => []), | ||
| args.conflictService?.getBatchAssessment().catch(() => null) ?? Promise.resolve(null), | ||
| ]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
fd registerIpc.tsRepository: arul28/ADE
Length of output: 103
🏁 Script executed:
# Let's examine the code around lines 792-798
sed -n '785,805p' apps/desktop/src/main/services/ipc/registerIpc.tsRepository: arul28/ADE
Length of output: 1272
🏁 Script executed:
# Let's look at the service method signatures to understand if they could throw synchronously
# First, let's check the rebaseSuggestionService interface/type
rg -A 5 "listSuggestions" apps/desktop/src --type ts --type tsx | head -50Repository: arul28/ADE
Length of output: 81
🏁 Script executed:
# Let's search for the service method signatures
rg "listSuggestions|listStatuses|listStateSnapshots|getBatchAssessment" apps/desktop/src --type ts -A 3 | head -100Repository: arul28/ADE
Length of output: 9548
🏁 Script executed:
# Let's find the service definitions/interfaces
fd -e ts rebaseSuggestionService.ts | grep -v testRepository: arul28/ADE
Length of output: 117
🏁 Script executed:
# Let's examine the rebaseSuggestionService to understand method signatures
cat -n apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts | head -60Repository: arul28/ADE
Length of output: 2993
🏁 Script executed:
# Let's see the listSuggestions method definition
rg -A 20 "listSuggestions" apps/desktop/src/main/services/lanes/rebaseSuggestionService.ts | head -50Repository: arul28/ADE
Length of output: 2086
🏁 Script executed:
# Let's check autoRebaseService.listStatuses
fd -e ts autoRebaseService.ts | grep -v testRepository: arul28/ADE
Length of output: 111
🏁 Script executed:
# Let's look at the autoRebaseService.listStatuses method
rg -A 15 "listStatuses" apps/desktop/src/main/services/lanes/autoRebaseService.tsRepository: arul28/ADE
Length of output: 2032
🏁 Script executed:
# Let's check laneService.listStateSnapshots
fd -e ts laneService.ts | grep -v testRepository: arul28/ADE
Length of output: 105
🏁 Script executed:
# Let's look at the laneService.listStateSnapshots method
rg -A 20 "listStateSnapshots" apps/desktop/src/main/services/lanes/laneService.tsRepository: arul28/ADE
Length of output: 889
🏁 Script executed:
# Let's check the conflictService.getBatchAssessment method
fd -e ts conflictService.ts | grep -v testRepository: arul28/ADE
Length of output: 113
🏁 Script executed:
# Let's look at the conflictService.getBatchAssessment method
rg -A 20 "getBatchAssessment" apps/desktop/src/main/services/conflicts/conflictService.tsRepository: arul28/ADE
Length of output: 1295
🏁 Script executed:
# Let's check the exact signature of listStateSnapshots - is it async or not?
rg -B 5 "listStateSnapshots()" apps/desktop/src/main/services/lanes/laneService.ts | head -20Repository: arul28/ADE
Length of output: 238
🏁 Script executed:
# Let's verify the full implementation of listStateSnapshots to confirm it's synchronous
rg -B 2 -A 15 "listStateSnapshots\(\)" apps/desktop/src/main/services/lanes/laneService.tsRepository: arul28/ADE
Length of output: 652
🏁 Script executed:
# Let's also check what the current implementation looks like in registerIpc.ts more carefully
# Looking for all Promise.resolve patterns in the file
rg "Promise.resolve" apps/desktop/src/main/services/ipc/registerIpc.ts -B 2 -A 2Repository: arul28/ADE
Length of output: 1023
Wrap function calls, not promises, to catch synchronous throws.
Promise.resolve(service.call()).catch(...) only catches rejected promises. The methods listSuggestions(), listStatuses(), listStateSnapshots(), and getBatchAssessment() can throw synchronously during execution (e.g., db.all() or db.get() calls within them), which would escape the catch handler and cause Promise.all to reject entirely instead of degrading gracefully.
Suggested fix
async function buildLaneListSnapshots(
args: Pick<AppContext, "laneService" | "sessionService" | "ptyService" | "agentChatService" | "rebaseSuggestionService" | "autoRebaseService" | "conflictService">,
lanes: LaneSummary[],
): Promise<LaneListSnapshot[]> {
+ const safe = <T>(load: () => Promise<T> | T, fallback: T): Promise<T> =>
+ Promise.resolve()
+ .then(load)
+ .catch(() => fallback);
+
const [sessions, rebaseSuggestions, autoRebaseStatuses, stateSnapshots, batchAssessment] = await Promise.all([
enrichSessionsForLaneList(args),
- Promise.resolve(args.rebaseSuggestionService?.listSuggestions() ?? []).catch(() => []),
- Promise.resolve(args.autoRebaseService?.listStatuses() ?? []).catch(() => []),
- Promise.resolve(args.laneService.listStateSnapshots()).catch(() => []),
- args.conflictService?.getBatchAssessment().catch(() => null) ?? Promise.resolve(null),
+ safe(() => args.rebaseSuggestionService?.listSuggestions() ?? [], []),
+ safe(() => args.autoRebaseService?.listStatuses() ?? [], []),
+ safe(() => args.laneService.listStateSnapshots(), []),
+ safe(() => args.conflictService?.getBatchAssessment() ?? null, null),
]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts` around lines 792 - 798,
The Promise.resolve(...) wrappers won't catch synchronous throws from service
methods; update each element in the Promise.all array to call the service inside
an async IIFE with try/catch so both sync and async errors are handled. For
example, replace Promise.resolve(args.rebaseSuggestionService?.listSuggestions()
?? []).catch(() => []) with (async () => { try { return await
args.rebaseSuggestionService?.listSuggestions() ?? []; } catch { return []; }
})(), and do the same pattern for autoRebaseService.listStatuses(),
laneService.listStateSnapshots(), and conflictService.getBatchAssessment(); keep
enrichSessionsForLaneList(...) as-is.
Comment on lines
69
to
+75
| function openRawDatabase(dbPath: string): DatabaseSyncType { | ||
| ensureParentDir(dbPath); | ||
| return new DatabaseSync(dbPath, { allowExtension: true }); | ||
| const db = new DatabaseSync(dbPath, { allowExtension: true }); | ||
| // Allow concurrent access from multiple ADE processes (e.g. dogfooding). | ||
| // Without this, a second instance gets SQLITE_BUSY immediately on writes. | ||
| db.exec("PRAGMA busy_timeout = 5000"); | ||
| return db; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which callsites bypass shared DB-open defaults.
# Expected: no production callsites should use direct `new DatabaseSync(...)`
# unless they also apply equivalent PRAGMAs (busy_timeout, etc).
rg -nP --type=ts -C2 '\bnew\s+DatabaseSync\s*\('
rg -nP --type=ts -C2 '\bopenRawDatabase\s*\('Repository: arul28/ADE
Length of output: 4983
Busy-timeout hardening is incomplete—fix direct DatabaseSync instantiation in recentProjectSummary.ts
The PRAGMA busy_timeout at kvDb.ts:74 applies only to calls through openRawDatabase, but recentProjectSummary.ts:44 creates DatabaseSync directly without this pragma. This split behavior means that path can still fail fast under lock contention while other codepaths tolerate it. Apply the same pragma there or refactor to use the shared helper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/state/kvDb.ts` around lines 69 - 75, The code
creates a DatabaseSync directly in recentProjectSummary (via new
DatabaseSync(...)) which bypasses the busy_timeout set in openRawDatabase;
update recentProjectSummary to either call openRawDatabase(dbPath) instead of
new DatabaseSync(...) or, if keeping direct instantiation, call db.exec("PRAGMA
busy_timeout = 5000") on that DatabaseSync instance immediately after creation
so it uses the same lock-wait behavior as openRawDatabase.
clearCache() was deadlocking by synchronously awaiting an in-flight pipeline load it had just invalidated — let it settle in the background instead since dispose() already marks the load as stale. Also fixes: MCP socket path scoped per project context, rev-parse prefers remote-tracking ref, busy_timeout on recentProjectSummary DB, lane refresh race on project switch, awaiting-input lanes sort first, send-button re-entrancy guard, transcript fetch race token, diff save nil guard, reasoning effort reset on model change, and misc iOS fixes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
apps/desktop/src/main/services/memory/embeddingService.ts (1)
565-588:⚠️ Potential issue | 🟠 MajorDon't treat cache clear as finished before repair can safely restart.
clearCache()now resolves afterpendingLoad.catch(...), but the repair path inapps/desktop/src/main/services/ipc/registerIpc.tsstarts a freshpreload({ forceRetry: true })immediately afterawait clearCache(). That lets the stale and repair pipelines overlap on the sameinstallPath, and a failedrm()is only logged, so the caller can't tell whether the cache is actually clear. Queue the retry off the stale-load settlement, or surface a failure instead of reporting a completed clear here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/memory/embeddingService.ts` around lines 565 - 588, clearCache currently returns before the stale extractorPromise has truly settled (it calls void pendingLoad.catch(...)) which allows concurrent repair/preload to run against the same installPath; change clearCache to wait for the pendingLoad to fully settle (await pendingLoad.catch(...) or await pendingLoad.then(...).catch(...)) or rethrow the pendingLoad error so callers (e.g., preload with forceRetry) know the clear failed; specifically update the clearCache function (references: clearCache, extractorPromise, dispose, installPath, fs.promises.rm, refreshCachedInstall, emitStatus) to not swallow pendingLoad errors and to await the pendingLoad completion before resolving so repair won't overlap the stale load.apps/desktop/src/main/main.ts (1)
2298-2303:⚠️ Potential issue | 🟠 MajorKeep
ADE_MCP_SOCKET_PATHisolated for the whole dogfood process.
scripts/dogfood.shsets this env var to keep the second ADE instance off the primary socket, but this branch only honors it whileprojectContexts.size === 0. As soon as that process opens another project context, new sockets fall back to<project>/.ade/mcp.sockand can collide with the primary instance again. Derive a per-project socket path from the override instead of dropping back to the default project path. Based on learnings: For ADE MCP changes, verify both headless MCP mode and the desktop socket-backed MCP path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/main.ts` around lines 2298 - 2303, The branch currently only uses ADE_MCP_SOCKET_PATH when projectContexts.size === 0, causing subsequent project contexts to fall back to adePaths.socketPath and collide; change the logic in the mcp socket selection so that if ADE_MCP_SOCKET_PATH (envSocketOverride) is present you always base the socket path off it instead of reverting to adePaths.socketPath — derive a per-project socket name from the override (e.g., append or interpolate a unique project identifier) when computing mcpSocketPath for each project context so each project gets a distinct socket while honoring the env override (update the variables envSocketOverride, mcpSocketPath and the code that uses projectContexts and adePaths.socketPath accordingly).
🧹 Nitpick comments (6)
apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift (2)
107-111: Consider debouncing rapidlocalStateRevisionchanges.The
.task(id: syncService.localStateRevision)modifier triggersloadDetail(refreshRemote: false)on every revision change. If the database emits changes rapidly (e.g., during a sync batch), this could cause multiple concurrent loads. ThebusyActionguard helps, but cached loads may still pile up.💡 Optional: Add a small debounce
.task(id: syncService.localStateRevision) { guard busyAction == nil else { return } + try? await Task.sleep(nanoseconds: 100_000_000) // 100ms debounce + guard !Task.isCancelled else { return } await loadDetail(refreshRemote: false) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift` around lines 107 - 111, The .task(id: syncService.localStateRevision) on LaneDetailScreen fires loadDetail(refreshRemote: false) for every localStateRevision change and can queue cached loads even with the busyAction guard; debounce rapid revisions before calling loadDetail by delaying execution and cancelling intermediate tasks (e.g., wrap the body in a Task that awaits Task.sleep(nanoseconds:) for a short interval and checks the latest syncService.localStateRevision and busyAction before calling loadDetail, or use a lightweight AsyncDebouncer helper) so only the final revision in a burst triggers loadDetail(refreshRemote: false); update the task block around loadDetail(refreshRemote: false) and reference syncService.localStateRevision, busyAction and loadDetail to implement the debounce/cancellation.
14-27: Non-private@Stateproperties appear intentional for extension access.SwiftLint flags these as warnings, but several properties (
detail,errorMessage,busyAction,selectedDiffRequest,commitMessage,amendCommit,stashMessage,confirmDiscardFile) are accessed by extensions in other files (e.g.,LaneDetailGitSection.swift). Swift extensions in the same module can accessinternalmembers but notprivateones.If extension access is required, consider using
private(set)where only reads are needed externally, or suppress the SwiftLint warning with an inline comment explaining the design decision.💡 Example for properties only read by extensions
- `@State` var errorMessage: String? + // swiftlint:disable:next private_swiftui_state + `@State` var errorMessage: String? // Accessed by LaneDetailGitSection extensionOr for properties that extensions only read:
- `@State` private(set) var detail: LaneDetailPayload? + `@State` private(set) var detail: LaneDetailPayload? // Already correct🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift` around lines 14 - 27, Several `@State` properties on LaneDetailScreen are intentionally accessed from extensions (e.g., LaneDetailGitSection) but SwiftLint flags them as non-private; update their visibility to private(set) for any properties only read by extensions (e.g., set private(set) on selectedDiffRequest, commitMessage, stashMessage, confirmDiscardFile, errorMessage if they are only read externally) and keep plain internal var for any properties that need to be mutated by extensions (leave amendCommit or others as-is if extensions write them). For properties that must remain mutable but trigger SwiftLint, add an inline SwiftLint suppression comment with a brief rationale above the property in LaneDetailScreen; ensure you reference the class name LaneDetailScreen and the extension file LaneDetailGitSection when deciding which properties are read-only vs writable.apps/desktop/src/renderer/components/lanes/LanesPage.tsx (2)
927-932: Simplify single-promisePromise.allSettledwrapper.With only one promise,
Promise.allSettledadds unnecessary complexity. The loop checking for partial failures also becomes redundant.♻️ Suggested simplification
- const results = await Promise.allSettled([refreshLanes()]); - for (const r of results) { - if (r.status === "rejected") { - console.error("Lane refresh partially failed:", r.reason); - } - } + try { + await refreshLanes(); + } catch (err) { + console.error("Lane refresh failed:", err); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx` around lines 927 - 932, The Promise.allSettled wrapper around a single call to refreshLanes() is unnecessary; replace "const results = await Promise.allSettled([refreshLanes()]);" and the subsequent for-loop checking results with a single await refreshLanes() call and handle errors with a try/catch that logs failures (e.g., console.error("Lane refresh failed:", error)); update the code in LanesPage.tsx where refreshLanes() is invoked to remove the results variable and loop and use try/catch around await refreshLanes() instead.
420-434: Consider logging errors instead of silently catching.The empty
.catch(() => {})swallows errors fromgetBatchAssessment(), which could mask underlying issues during development. Even a minimalconsole.errorwould aid debugging.🔧 Suggested improvement
- }).catch(() => {}); + }).catch((err) => { + console.error("Failed to refresh conflict status:", err); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx` around lines 420 - 434, The empty catch on the promise from window.ade.conflicts.getBatchAssessment() swallows errors; modify the catch to accept an error parameter and log it (e.g., console.error or the app logger) with context before returning so failures are visible during development. Locate the promise chain around getBatchAssessment() inside LanesPage (where conflictByLaneId is built and useAppStore.setState is called) and replace .catch(() => {}) with .catch((err) => { console.error("getBatchAssessment failed:", err); }); or the equivalent app logging call.apps/desktop/src/main/services/ipc/registerIpc.ts (2)
6327-6339: Use the shared empty snapshot helper for the IPC fallback.This redefines the
AutoUpdateSnapshotshape inline, so a future field addition can update the service helper but leave this IPC path stale. ImportingcreateEmptyAutoUpdateSnapshot()here keeps the fallback and renderer contract in lockstep.As per coding guidelines "Keep IPC contracts, preload types, shared types, and renderer usage in sync whenever an interface changes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ipc/registerIpc.ts` around lines 6327 - 6339, Replace the inline hard-coded AutoUpdateSnapshot fallback in the IPC handler for IPC.updateGetState with the shared helper so the contract stays in sync: when handling IPC.updateGetState (the ipcMain.handle callback using getCtx().autoUpdateService?.getSnapshot()), import and call createEmptyAutoUpdateSnapshot() as the nullish fallback instead of constructing the object literal so any future fields added to AutoUpdateSnapshot are picked up automatically.
755-785: Drive both session IPCs through one enrichment helper.This reimplements the identity-chat filtering and runtime-state merge logic that already exists in
IPC.sessionsList, so the snapshot IPC and session list can drift again on the next change. Consider parameterizing this helper with the list args and reusing it from both handlers.As per coding guidelines "Keep IPC contracts, preload types, shared types, and renderer usage in sync whenever an interface changes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ipc/registerIpc.ts` around lines 755 - 785, The helper enrichSessionsForLaneList duplicates the identity-chat filtering and runtime-state merge logic already implemented in IPC.sessionsList; refactor by extracting a single parameterized enrichment function (e.g., enrichSessionsForLaneList -> enrichSessionsWithChats or similar) that accepts the shared dependencies and the session-listing arguments (sessionService, ptyService, agentChatService, plus any list options) and returns TerminalSessionSummary[]; update both IPC.sessionsList and the snapshot IPC to call that single helper so identitySessionIds filtering, chat summary mapping, and runtimeState merging (awaitingInput/running/idle) are centralized and no logic is duplicated. Ensure you reference and replace usages of enrichSessionsForLaneList and IPC.sessionsList to use the new helper and preserve existing runtimeState value assignments and isChatToolType checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 1597-1601: The branch that assigns parentLaneId (based on
mainScore, bestScore, explicitParentLaneId and bestLaneId) can end up persisting
bestLaneId as parent_lane_id for a lane that is actually the detected primary;
update the logic in the parent assignment and before the DB insert that writes
parent_lane_id so that if parentLaneId === bestLaneId (i.e., the detected
primary was selected as the parent) you normalize parentLaneId to null; ensure
this check is applied where parentLaneId is set (refer to parentLaneId,
bestLaneId, explicitParentLaneId, mainScore, bestScore) and again right before
persisting parent_lane_id so the insert never writes a self/primary parent id.
In `@apps/ios/ADE/Views/Lanes/LaneDiffScreen.swift`:
- Around line 163-182: The early return in load() when request.path is nil
leaves the UI ambiguous; update load() to handle the missing path explicitly by
setting diff = nil and editedText = "" (or appropriate empty values), ensure
isLoading is properly toggled (set true at start and false before returning),
and either throw a specific error (e.g., a new LoadError.missingPath) or set an
explicit UI state flag (e.g., missingPath/emptyState) so the view can render an
informative empty/error state; update references to request.path, isLoading,
diff, editedText, and load() accordingly.
In `@apps/ios/ADE/Views/Lanes/LaneHelpers.swift`:
- Around line 101-104: The helper laneListEmptyStateMessage currently checks the
raw searchText which treats whitespace-only queries as non-empty; update it to
trim searchText using trimmingCharacters(in: .whitespacesAndNewlines)
(consistent with laneMatchesSearch) and use the trimmed value when deciding to
return the "search" empty-state copy so that queries like " " are treated as
empty and do not show the search-specific message.
- Around line 64-66: The comparison currently orders lanes by the raw createdAt
strings (lhs.lane.createdAt / rhs.lane.createdAt); change this to parse those
ISO8601 timestamps into Date objects and compare the Dates instead. Add a helper
like parsedLaneTimestamp(_ rawValue: String) -> Date? that tries
cachedISO8601Formatter.date(from:) then
cachedISO8601FormatterNoFractional.date(from:), use it in the comparison (parse
lhs and rhs, if both parse compare the Date values, if one or both fail fall
back to the existing string comparison) so fractional and non‑fractional ISO8601
forms sort chronologically.
- Around line 61-63: The sort helper laneListSortSnapshots currently only
prioritizes lanes with laneType == "primary", removing the previous higher
priority for lanes needing user input; update laneListSortSnapshots to also
check lane.laneState == "awaiting-input" (use the
LaneListSnapshot.lane.laneState property) so that if one snapshot has state
"awaiting-input" and the other does not it sorts before the non-awaiting one;
keep the existing primary checks as-is and add the awaiting-input comparisons
immediately after them (also add the symmetric check to return false when rhs is
awaiting-input and lhs is not) so awaiting-input lanes surface above other
non-actionable lanes.
In `@scripts/dogfood.sh`:
- Around line 18-27: The find call can fail when the .ade/worktrees directory is
absent under MAIN_ROOT, causing the script to abort under set -e; before running
find in the block that sets LANE_NAME and WORKTREE_DIR, explicitly test that
"$MAIN_ROOT/.ade/worktrees" exists and is a directory and if not print the same
"No worktree found..." and "Available:" messages (or a single "(none)") and exit
1; then change the find invocation that sets WORKTREE_DIR to use find
"$MAIN_ROOT/.ade/worktrees" -maxdepth 1 -type d -name "*${LANE_NAME}*" -print
-quit so it stops after the first match.
---
Duplicate comments:
In `@apps/desktop/src/main/main.ts`:
- Around line 2298-2303: The branch currently only uses ADE_MCP_SOCKET_PATH when
projectContexts.size === 0, causing subsequent project contexts to fall back to
adePaths.socketPath and collide; change the logic in the mcp socket selection so
that if ADE_MCP_SOCKET_PATH (envSocketOverride) is present you always base the
socket path off it instead of reverting to adePaths.socketPath — derive a
per-project socket name from the override (e.g., append or interpolate a unique
project identifier) when computing mcpSocketPath for each project context so
each project gets a distinct socket while honoring the env override (update the
variables envSocketOverride, mcpSocketPath and the code that uses
projectContexts and adePaths.socketPath accordingly).
In `@apps/desktop/src/main/services/memory/embeddingService.ts`:
- Around line 565-588: clearCache currently returns before the stale
extractorPromise has truly settled (it calls void pendingLoad.catch(...)) which
allows concurrent repair/preload to run against the same installPath; change
clearCache to wait for the pendingLoad to fully settle (await
pendingLoad.catch(...) or await pendingLoad.then(...).catch(...)) or rethrow the
pendingLoad error so callers (e.g., preload with forceRetry) know the clear
failed; specifically update the clearCache function (references: clearCache,
extractorPromise, dispose, installPath, fs.promises.rm, refreshCachedInstall,
emitStatus) to not swallow pendingLoad errors and to await the pendingLoad
completion before resolving so repair won't overlap the stale load.
---
Nitpick comments:
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 6327-6339: Replace the inline hard-coded AutoUpdateSnapshot
fallback in the IPC handler for IPC.updateGetState with the shared helper so the
contract stays in sync: when handling IPC.updateGetState (the ipcMain.handle
callback using getCtx().autoUpdateService?.getSnapshot()), import and call
createEmptyAutoUpdateSnapshot() as the nullish fallback instead of constructing
the object literal so any future fields added to AutoUpdateSnapshot are picked
up automatically.
- Around line 755-785: The helper enrichSessionsForLaneList duplicates the
identity-chat filtering and runtime-state merge logic already implemented in
IPC.sessionsList; refactor by extracting a single parameterized enrichment
function (e.g., enrichSessionsForLaneList -> enrichSessionsWithChats or similar)
that accepts the shared dependencies and the session-listing arguments
(sessionService, ptyService, agentChatService, plus any list options) and
returns TerminalSessionSummary[]; update both IPC.sessionsList and the snapshot
IPC to call that single helper so identitySessionIds filtering, chat summary
mapping, and runtimeState merging (awaitingInput/running/idle) are centralized
and no logic is duplicated. Ensure you reference and replace usages of
enrichSessionsForLaneList and IPC.sessionsList to use the new helper and
preserve existing runtimeState value assignments and isChatToolType checks.
In `@apps/desktop/src/renderer/components/lanes/LanesPage.tsx`:
- Around line 927-932: The Promise.allSettled wrapper around a single call to
refreshLanes() is unnecessary; replace "const results = await
Promise.allSettled([refreshLanes()]);" and the subsequent for-loop checking
results with a single await refreshLanes() call and handle errors with a
try/catch that logs failures (e.g., console.error("Lane refresh failed:",
error)); update the code in LanesPage.tsx where refreshLanes() is invoked to
remove the results variable and loop and use try/catch around await
refreshLanes() instead.
- Around line 420-434: The empty catch on the promise from
window.ade.conflicts.getBatchAssessment() swallows errors; modify the catch to
accept an error parameter and log it (e.g., console.error or the app logger)
with context before returning so failures are visible during development. Locate
the promise chain around getBatchAssessment() inside LanesPage (where
conflictByLaneId is built and useAppStore.setState is called) and replace
.catch(() => {}) with .catch((err) => { console.error("getBatchAssessment
failed:", err); }); or the equivalent app logging call.
In `@apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift`:
- Around line 107-111: The .task(id: syncService.localStateRevision) on
LaneDetailScreen fires loadDetail(refreshRemote: false) for every
localStateRevision change and can queue cached loads even with the busyAction
guard; debounce rapid revisions before calling loadDetail by delaying execution
and cancelling intermediate tasks (e.g., wrap the body in a Task that awaits
Task.sleep(nanoseconds:) for a short interval and checks the latest
syncService.localStateRevision and busyAction before calling loadDetail, or use
a lightweight AsyncDebouncer helper) so only the final revision in a burst
triggers loadDetail(refreshRemote: false); update the task block around
loadDetail(refreshRemote: false) and reference syncService.localStateRevision,
busyAction and loadDetail to implement the debounce/cancellation.
- Around line 14-27: Several `@State` properties on LaneDetailScreen are
intentionally accessed from extensions (e.g., LaneDetailGitSection) but
SwiftLint flags them as non-private; update their visibility to private(set) for
any properties only read by extensions (e.g., set private(set) on
selectedDiffRequest, commitMessage, stashMessage, confirmDiscardFile,
errorMessage if they are only read externally) and keep plain internal var for
any properties that need to be mutated by extensions (leave amendCommit or
others as-is if extensions write them). For properties that must remain mutable
but trigger SwiftLint, add an inline SwiftLint suppression comment with a brief
rationale above the property in LaneDetailScreen; ensure you reference the class
name LaneDetailScreen and the extension file LaneDetailGitSection when deciding
which properties are read-only vs writable.
🪄 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: 408ef92b-e234-40e3-964e-20cab605c9e1
⛔ Files ignored due to path filters (9)
changelog/v1.0.10.mdxis excluded by!changelog/**changelog/v1.0.11.mdxis excluded by!changelog/**changelog/v1.0.12.mdxis excluded by!changelog/**changelog/v1.0.3.mdxis excluded by!changelog/**changelog/v1.0.4.mdxis excluded by!changelog/**changelog/v1.0.6.mdxis excluded by!changelog/**changelog/v1.0.7.mdxis excluded by!changelog/**changelog/v1.0.8.mdxis excluded by!changelog/**docs.jsonis excluded by!docs.json
📒 Files selected for processing (27)
apps/desktop/src/main/main.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/memory/embeddingService.tsapps/desktop/src/main/services/projects/recentProjectSummary.tsapps/desktop/src/main/services/state/globalState.tsapps/desktop/src/main/services/updates/autoUpdateService.test.tsapps/desktop/src/main/services/updates/autoUpdateService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/AutoUpdateControl.test.tsxapps/desktop/src/renderer/components/app/AutoUpdateControl.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/onboarding/DevToolsSection.tsxapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/core.tsapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Lanes/LaneChatLaunchSheet.swiftapps/ios/ADE/Views/Lanes/LaneChatSessionView.swiftapps/ios/ADE/Views/Lanes/LaneDetailScreen.swiftapps/ios/ADE/Views/Lanes/LaneDiffScreen.swiftapps/ios/ADE/Views/Lanes/LaneHelpers.swiftapps/ios/ADE/Views/Lanes/LaneListViewParts.swiftscripts/dogfood.sh
✅ Files skipped from review due to trivial changes (3)
- apps/desktop/src/renderer/components/onboarding/DevToolsSection.tsx
- apps/desktop/src/shared/types/core.ts
- apps/ios/ADE/Views/Lanes/LaneChatLaunchSheet.swift
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/src/shared/ipc.ts
- apps/desktop/src/preload/global.d.ts
- apps/desktop/src/renderer/browserMock.ts
- apps/ios/ADE/Views/Lanes/LaneChatSessionView.swift
- apps/ios/ADE/Views/Lanes/LaneListViewParts.swift
Comment on lines
+1597
to
+1601
| if (mainScore <= bestScore) { | ||
| // The branch is based on main (or closer to it), no parent lane | ||
| // — unless the caller explicitly provided one. | ||
| parentLaneId = explicitParentLaneId ?? null; | ||
| } else { |
There was a problem hiding this comment.
Normalize detected primary parent to null before insert.
When detection picks the primary lane as bestLaneId, Line 1608 persists that ID as parent_lane_id (Line 1635). That creates an invalid parented-root lane state until a later repair run.
Suggested fix
- const parent = parentLaneId ? getLaneRow(parentLaneId) : null;
- if (parentLaneId && !parent) throw new Error(`Parent lane not found: ${parentLaneId}`);
- if (parent && parent.status === "archived") throw new Error("Parent lane is archived");
+ const detectedParent = parentLaneId ? getLaneRow(parentLaneId) : null;
+ if (parentLaneId && !detectedParent) throw new Error(`Parent lane not found: ${parentLaneId}`);
+ const parent = detectedParent?.lane_type === "primary" ? null : detectedParent;
+ if (parent && parent.status === "archived") throw new Error("Parent lane is archived");
@@
- [laneId, projectId, displayName, args.description ?? null, baseRef, branchRef, worktreePath, parentLaneId, now]
+ [laneId, projectId, displayName, args.description ?? null, baseRef, branchRef, worktreePath, parent?.id ?? null, now]Also applies to: 1608-1609, 1617-1636
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/lanes/laneService.ts` around lines 1597 -
1601, The branch that assigns parentLaneId (based on mainScore, bestScore,
explicitParentLaneId and bestLaneId) can end up persisting bestLaneId as
parent_lane_id for a lane that is actually the detected primary; update the
logic in the parent assignment and before the DB insert that writes
parent_lane_id so that if parentLaneId === bestLaneId (i.e., the detected
primary was selected as the parent) you normalize parentLaneId to null; ensure
this check is applied where parentLaneId is set (refer to parentLaneId,
bestLaneId, explicitParentLaneId, mainScore, bestScore) and again right before
persisting parent_lane_id so the insert never writes a self/primary parent id.
Comment on lines
+163
to
+182
| @MainActor | ||
| private func load() async throws { | ||
| guard let path = request.path else { return } | ||
| isLoading = true | ||
| defer { isLoading = false } | ||
| do { | ||
| let loaded = try await syncService.fetchFileDiff( | ||
| laneId: request.laneId, | ||
| path: path, | ||
| mode: request.mode, | ||
| compareRef: request.compareRef, | ||
| compareTo: request.compareTo | ||
| ) | ||
| diff = loaded | ||
| editedText = loaded.modified.text | ||
| } catch { | ||
| diff = nil | ||
| throw error | ||
| } | ||
| } |
There was a problem hiding this comment.
Early return when path is nil leaves UI in ambiguous state.
When request.path is nil, load() returns immediately without setting isLoading = true or throwing an error. The diff remains nil, and isLoading stays false, causing the view to render neither the ProgressView nor the diff content — effectively showing an empty screen below the header.
Consider showing an informative empty state or error message when path is unavailable.
💡 Suggested improvement
`@MainActor`
private func load() async throws {
- guard let path = request.path else { return }
+ guard let path = request.path else {
+ errorMessage = "No file path available for this diff."
+ return
+ }
isLoading = true
defer { isLoading = false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Views/Lanes/LaneDiffScreen.swift` around lines 163 - 182, The
early return in load() when request.path is nil leaves the UI ambiguous; update
load() to handle the missing path explicitly by setting diff = nil and
editedText = "" (or appropriate empty values), ensure isLoading is properly
toggled (set true at start and false before returning), and either throw a
specific error (e.g., a new LoadError.missingPath) or set an explicit UI state
flag (e.g., missingPath/emptyState) so the view can render an informative
empty/error state; update references to request.path, isLoading, diff,
editedText, and load() accordingly.
Per-project MCP socket derivation from env override, use shared createEmptyAutoUpdateSnapshot helper, simplify single-element Promise.allSettled, log getBatchAssessment errors, guard dogfood.sh against missing worktrees dir, explicit nil-path handling in LaneDiffScreen, awaiting-input sort priority and proper ISO8601 date parsing in iOS lane helpers, and trim whitespace-only search queries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 9, 2026
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 by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation