Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughThis PR removes context document generation and auto-refresh functionality from the codebase, including deletion of context builder and service modules, removal of IPC/preload APIs, UI components, configuration types, and documentation. System guidance for ADE CLI is updated to reflect direct repository inspection instead of auto-generated context docs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Here's the review. No fixes applied per your request. Commit 1 — Remove context pack / contextDoc system (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
closeExistingPeersForDevice and the getPeerStates deduplication are a peer-management bug fix, not related to context doc generation. Bundling them here makes it harder to bisect a regression if the sync behavior changes. Consider splitting into a separate PR for reviewability — or at least add an inline comment explaining what bug this addresses.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/sync/syncHostService.ts
Line: 1691-1693
Comment:
**Unrelated fix bundled into context-doc removal**
`closeExistingPeersForDevice` and the `getPeerStates` deduplication are a peer-management bug fix, not related to context doc generation. Bundling them here makes it harder to bisect a regression if the sync behavior changes. Consider splitting into a separate PR for reviewability — or at least add an inline comment explaining what bug this addresses.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
configuration/settings.mdx (1)
314-314:⚠️ Potential issue | 🟡 MinorStale reference to removed “Auto-refresh events” setting.
This PR removes context-doc auto-refresh (per-event toggles for session end / commit / PR create / mission start, etc.) from the Workspace section, but the Solo Developer recommended-settings tab still suggests “Auto-refresh events: On PR create + On mission start.” Remove this bullet (or replace it with a still-supported setting) so the recommendations don’t reference a deleted feature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configuration/settings.mdx` at line 314, Remove the stale recommendation "**Auto-refresh events: On PR create + On mission start**" from the Solo Developer recommended-settings section (the bullet text shown in the diff) because the context-doc auto-refresh feature was removed; either delete this bullet entirely or replace it with a currently-supported setting (e.g., another existing Workspace toggle) and ensure the surrounding markdown in settings.mdx updates accordingly so the recommendations no longer reference the deleted feature.lanes/packs.mdx (2)
124-124:⚠️ Potential issue | 🟡 MinorStale
Settings > Contextreference.
configuration/settings.mdxno longer contains a "Context" settings section in this PR (context-doc settings, including auto-refresh/retention preferences, were removed). The pointer to "Settings > Context > Pack History Retention" therefore links readers to UI that no longer exists. Update or remove this sentence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lanes/packs.mdx` at line 124, The sentence in lanes/packs.mdx referencing "Settings > Context > Pack History Retention" is stale; open lanes/packs.mdx, locate the paragraph that contains "Settings > Context > Pack History Retention" (the Pack files description) and either remove the broken UI pointer or replace it with the correct/current location or wording (e.g., a generic note that history retention is configurable in Settings or link to the updated configuration docs). Update the text so it no longer points to the nonexistent "Settings > Context" section.
1-9:⚠️ Potential issue | 🟠 MajorNew Lane Pack definition contradicts the rest of the doc.
Line 9 reframes a Lane Pack as a static "structured lane briefing," but the frontmatter
description(line 3) still calls them "auto-generated, versioned context bundles," and the rest of the doc (Pack Freshness, automatic regeneration on commit, green/amber/red freshness indicators, automatic context injection at session start, Project/Feature/Conflict packs, history retention) still documents the very auto-generation/injection machinery this PR is removing. Please reconcile — either keep the auto-generation narrative if Lane Packs still work that way, or strip the now-stale sections (Pack Freshness, Context Injection, etc.) so readers aren’t pointed at features that no longer exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lanes/packs.mdx` around lines 1 - 9, The doc currently contradicts itself: the frontmatter description says Lane Packs are "auto-generated, versioned context bundles" but the opening sentence calls them a static "structured lane briefing," while several sections (Pack Freshness, Context Injection, automatic regeneration on commit, green/amber/red indicators, Project/Feature/Conflict packs, history retention) still describe auto-generation/injection behavior that this PR removes. Decide which model to keep and reconcile the content: either (A) restore the auto-generation narrative by reverting the static wording in the opening paragraph and ensuring the frontmatter `description` and sections like "Pack Freshness", "Context Injection", and mentions of automatic regeneration and freshness indicators remain and align with terms "Lane Pack", "Pack Freshness", and "Context Injection"; or (B) remove/strip all stale auto-generation/injection sections (Pack Freshness, Context Injection, automatic regeneration on commit, freshness indicators, Project/Feature/Conflict packs, history retention) and update the frontmatter `description` and the opening paragraph to clearly define a static "structured lane briefing" and any new behavior. Apply the chosen change consistently across the document so the frontmatter, the opening "What is a Lane Pack?" paragraph, and all headings use the same model.apps/desktop/src/renderer/components/app/AppShell.tsx (1)
179-183:⚠️ Potential issue | 🟡 MinorRemove dead
aiFailure/aiMockProviderstate and banners.Both
setAiFailureandsetAiMockProviderare only called withnullvalues (lines 640, 935). No code path in the codebase sets these to non-null values, making the banners at lines 901–908 and 910–942 unreachable dead code. These were likely orphaned when the context-doc/AI-job event stream was removed. Delete the unused state declarations, setters, and associated JSX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/app/AppShell.tsx` around lines 179 - 183, Remove the dead AI banner state and UI: delete the unused state declarations aiFailure and aiMockProvider (and their types AiBannerState and the inline createdAt shape) and remove any calls to their setters setAiFailure and setAiMockProvider, then remove the JSX that renders the AI banners tied to those states inside the AppShell component (the unreachable banner blocks). Keep aiStatus and related AI settings logic if still used. Ensure imports/types only referenced by the removed code are also cleaned up.
🧹 Nitpick comments (10)
apps/desktop/src/shared/adeCliGuidance.ts (1)
1-12: Deduplicate the two guidance strings to prevent drift.
ADE_CLI_AGENT_GUIDANCEandADE_CLI_INLINE_GUIDANCEnow restate the same six directives in two formats, and small wording differences are already creeping in (e.g., array form: "fall back tonode apps/ade-cli/dist/cli.cjs ...after confirming the file exists" vs inline form: "after confirming it exists"). Future edits to one are easy to miss in the other and the tests asserting specific phrases in both forms won't catch semantic drift. Consider a single source-of-truth array and derive the inline form from it.♻️ Proposed refactor
-export const ADE_CLI_AGENT_GUIDANCE = [ - "## ADE CLI", - "`ade` is the default control plane for ADE-managed sessions: lanes, missions, PRs, chats/sessions, memory, proof, config, and process state.", - "If `command -v ade` fails, try `${ADE_CLI_PATH:-}` when set, then `${ADE_CLI_BIN_DIR:-}/ade`, and in an ADE source checkout fall back to `node apps/ade-cli/dist/cli.cjs ...` after confirming the file exists.", - "The only normal reason to skip ADE CLI for an ADE action is that the user truly does not have it installed or reachable after those fallbacks.", - "For ADE work beyond the immediate local edit, shell command, or repository inspection in front of you, check ADE CLI first: run `ade doctor` if needed, use typed commands like `ade lanes list --text` / `ade prs checks <pr> --text`, or discover with `ade actions list --text` and `ade actions run ...`.", - "When the user asks you to capture, send, attach, or provide proof, use whatever computer-use or browser tool is appropriate to produce the evidence, then register it with ADE via `ade proof ...` so it appears in the ADE proof drawer for the active chat, mission, or lane.", - "When you run processes of any kind, track what you started and clean up old, stale, or finished processes before leaving the task.", -].join("\n"); - -export const ADE_CLI_INLINE_GUIDANCE = - "`ade` is the default control plane for ADE-managed sessions: lanes, missions, PRs, chats/sessions, memory, proof, config, and process state. If `command -v ade` fails, try `${ADE_CLI_PATH:-}`, then `${ADE_CLI_BIN_DIR:-}/ade`, and in an ADE source checkout `node apps/ade-cli/dist/cli.cjs ...` after confirming it exists. The only normal reason to skip ADE CLI for an ADE action is that the user truly does not have it installed or reachable after those fallbacks. For ADE work beyond the immediate local edit, shell command, or repository inspection in front of you, check ADE CLI first: try `ade doctor`, typed `ade ... --text` commands, or `ade actions list --text` / `ade actions run ...`. When the user asks you to capture, send, attach, or provide proof, use whatever computer-use or browser tool is appropriate to produce the evidence, then register it with ADE via `ade proof ...` so it appears in the ADE proof drawer for the active chat, mission, or lane. When you run processes of any kind, track what you started and clean up old, stale, or finished processes before leaving the task."; +const ADE_CLI_GUIDANCE_BODY = [ + "`ade` is the default control plane for ADE-managed sessions: lanes, missions, PRs, chats/sessions, memory, proof, config, and process state.", + "If `command -v ade` fails, try `${ADE_CLI_PATH:-}` when set, then `${ADE_CLI_BIN_DIR:-}/ade`, and in an ADE source checkout fall back to `node apps/ade-cli/dist/cli.cjs ...` after confirming the file exists.", + "The only normal reason to skip ADE CLI for an ADE action is that the user truly does not have it installed or reachable after those fallbacks.", + "For ADE work beyond the immediate local edit, shell command, or repository inspection in front of you, check ADE CLI first: run `ade doctor` if needed, use typed commands like `ade lanes list --text` / `ade prs checks <pr> --text`, or discover with `ade actions list --text` and `ade actions run ...`.", + "When the user asks you to capture, send, attach, or provide proof, use whatever computer-use or browser tool is appropriate to produce the evidence, then register it with ADE via `ade proof ...` so it appears in the ADE proof drawer for the active chat, mission, or lane.", + "When you run processes of any kind, track what you started and clean up old, stale, or finished processes before leaving the task.", +] as const; + +export const ADE_CLI_AGENT_GUIDANCE = ["## ADE CLI", ...ADE_CLI_GUIDANCE_BODY].join("\n"); +export const ADE_CLI_INLINE_GUIDANCE = ADE_CLI_GUIDANCE_BODY.join(" ");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/shared/adeCliGuidance.ts` around lines 1 - 12, The two exports ADE_CLI_AGENT_GUIDANCE and ADE_CLI_INLINE_GUIDANCE duplicate the same guidance and risk drift; refactor by creating a single source-of-truth array (e.g., ADE_CLI_GUIDANCE_LINES) containing the guidance lines and derive ADE_CLI_AGENT_GUIDANCE via ADE_CLI_GUIDANCE_LINES.join("\n") and ADE_CLI_INLINE_GUIDANCE by joining the same array into a single inline sentence string (e.g., ADE_CLI_GUIDANCE_LINES.join(" ") with any minimal whitespace normalization), then replace the current duplicated literals with those derived values so all edits are made in one place.chat/capabilities.mdx (1)
30-31: Optional: icon no longer fits the card content.The
databaseicon was appropriate for the previous "Context pack queries" card but is semantically misaligned with "Repo inspection". Since the card describes inspection of repository docs, source files, file maps, and activity, a file/search-oriented icon would be more accurate. Any ofmagnifying-glass,folder-tree, orfile-magnifying-glass(all available in Font Awesome, Mintlify's default icon library) would better communicate the purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chat/capabilities.mdx` around lines 30 - 31, The Card with title "Repo inspection" currently uses icon="database" which is semantically mismatched; update the icon prop on that Card (Card title="Repo inspection" icon="database") to a file/search-oriented Font Awesome icon such as "magnifying-glass", "folder-tree", or "file-magnifying-glass" so the icon better reflects inspecting repository docs, source files, file maps, and activity.apps/desktop/src/renderer/components/chat/AgentChatPane.test.ts (1)
77-85: Outdated test description.The test name still says "when project docs are prepended" but project-doc prepending was removed in this PR (the very change being made on line 84 removes the previous
[Project Contextexpectation). Consider renaming so the test intent matches the new behavior.♻️ Suggested rename
- it("preserves the default attachment review request when project docs are prepended", () => { + it("uses the default attachment review request as both display and send text", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatPane.test.ts` around lines 77 - 85, The test name is outdated because project-doc prepending was removed; update the spec title to match current behavior (e.g., rename the it(...) description to "preserves the default attachment review request" or "preserves the default attachment review request when no project docs are prepended") so it accurately describes the assertion around buildParallelLaunchPrompt returning displayText and sendText equal to "Please review the attached files."; adjust only the test description string near the assertions referencing buildParallelLaunchPrompt, displayText, and sendText.apps/desktop/src/main/services/orchestrator/promptInspector.ts (1)
210-213: Minor inconsistency withbaseOrchestratorAdapter.tsformatting.These two sentences are pushed as separate
sectionsentries, so they end up paragraph-separated (\n\n) in the worker base guidance. InbaseOrchestratorAdapter.ts(lines 571–576) the same two sentences are joined with"\n"(single newline) within one push. Not a bug, but if the intent is for inspector preview to mirror the actual worker prompt, consider matching the formatting.♻️ Suggested alignment
- sections.push( - "You are working within ADE (Autonomous Development Environment), an Electron-based multi-agent development tool. ADE manages lanes (git worktrees), missions (task orchestration), PRs, and agent sessions.", - "Project orientation: do not assume generated PRD or architecture summaries exist. When you need product or architecture context, inspect the repo directly: start with AGENTS.md, README.md, docs/, package manifests, and the relevant source files in your lane worktree.", - ); + sections.push( + [ + "You are working within ADE (Autonomous Development Environment), an Electron-based multi-agent development tool. ADE manages lanes (git worktrees), missions (task orchestration), PRs, and agent sessions.", + "Project orientation: do not assume generated PRD or architecture summaries exist. When you need product or architecture context, inspect the repo directly: start with AGENTS.md, README.md, docs/, package manifests, and the relevant source files in your lane worktree.", + ].join("\n"), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/orchestrator/promptInspector.ts` around lines 210 - 213, The two guidance sentences added via sections.push in promptInspector.ts are split into separate array entries (causing a blank paragraph), which is inconsistent with the worker prompt where the same guidance is joined with a single newline; update the promptInspector.ts call to combine those two strings into a single sections.push entry using "\n" between them so its formatting matches the sections.push usage in baseOrchestratorAdapter (locate the sections.push call in promptInspector.ts and mirror the single-string-with-"\n" pattern used in baseOrchestratorAdapter).apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx (1)
1697-1697: Stale "context" mention in inline comment.Now that the project-context toggle (
includeProjectDocs/BookOpen) has been removed, the inline section header on Line 1697 still lists "context" among the right-side controls. Consider tidying it to match the actual rendered controls (attachment, commands, proof, send).♻️ Suggested tweak
- {/* Right: attachment, commands, proof, context, send */} + {/* Right: attachment, commands, proof, send */}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx` at line 1697, Update the stale inline comment in the AgentChatComposer component: remove the word "context" from the right-side controls comment and update it to accurately list the rendered controls (e.g., "Right: attachment, commands, proof, send") so the inline header near the comment in AgentChatComposer.tsx matches the actual UI elements.apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (2)
2934-3006: Minor:finalTextis now a redundant alias fortext.
finalTextwas previously the merge point for the project-context prelude. After removing that prelude,finalTextis initialized totextat line 2936 and never reassigned, so the alias just adds noise at theoptimisticEnvelope,steer, andsendcall sites. Consider inliningtextdirectly. Optional cleanup, not a behavior change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines 2934 - 3006, finalText is an unused alias of text in AgentChatPane; remove the let finalText = text declaration and replace all uses of finalText with text (notably inside optimisticEnvelope, the window.ade.agentChat.steer call, and the window.ade.agentChat.send call), and remove any now-unused variable declarations so AgentChatPane no longer contains the redundant finalText symbol.
702-716: Optional: collapsebuildParallelLaunchPromptnow thatsendText === displayText.With the project-context prelude gone, this helper always returns the same value for
sendTextanddisplayText. The function is still useful as a single source of truth for the empty-text + attachments fallback, but consider simplifying its return shape (e.g., just return astring) and updating the two destructuring sites. Not blocking — keeping the dual-field return preserves the parallel withagentChat.send'stext/displayTextparameters and minimizes call-site churn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines 702 - 716, buildParallelLaunchPrompt currently returns an object with sendText and displayText that are always identical; simplify it to return a single string (e.g., return the chosen text or empty string) and update all call sites that destructure { sendText, displayText } (search for buildParallelLaunchPrompt(...) usages) to accept a single string and map it to the appropriate parameters (e.g., sendText/displayText or directly pass to agentChat.send). Keep the fallback logic (trim input, use DEFAULT_PARALLEL_ATTACHMENT_REQUEST when attachmentCount > 0) inside buildParallelLaunchPrompt to preserve behavior.apps/desktop/src/main/services/sync/syncHostService.ts (2)
2160-2172: Dedup logic is correct; minor variable shadowing.The dedup-by-
deviceIdselecting the newestconnectedAtis sound —nowIso()produces ISO 8601, which is lexicographically comparable, and aligns with the existingpeerStates.find(...)consumer insyncService.ts:648-666. One readability nit: the innerpeerin the.map/.filtercallbacks and the outerfor (const peer of …)shadow each other with different types (PeerStatevsSyncPeerConnectionState), which is easy to misread.♻️ Optional readability tweak
getPeerStates(): SyncPeerConnectionState[] { const dbVersion = args.db.sync.getDbVersion(); const latestByDevice = new Map<string, SyncPeerConnectionState>(); - for (const peer of [...peers] - .map((peer) => toSyncPeerConnectionState(peer, dbVersion)) - .filter((peer): peer is SyncPeerConnectionState => peer != null)) { - const existing = latestByDevice.get(peer.deviceId); - if (!existing || peer.connectedAt > existing.connectedAt) { - latestByDevice.set(peer.deviceId, peer); + for (const candidate of [...peers] + .map((p) => toSyncPeerConnectionState(p, dbVersion)) + .filter((p): p is SyncPeerConnectionState => p != null)) { + const existing = latestByDevice.get(candidate.deviceId); + if (!existing || candidate.connectedAt > existing.connectedAt) { + latestByDevice.set(candidate.deviceId, candidate); } } return [...latestByDevice.values()]; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncHostService.ts` around lines 2160 - 2172, The loop in getPeerStates uses the identifier peer twice (once for PeerState in the map/filter chain that calls toSyncPeerConnectionState, and again for SyncPeerConnectionState in the for...of), causing variable shadowing that reduces readability; rename the inner lambda parameter (e.g., rawPeer or p) in the [...peers].map(...)/.filter(...) chain so the mapped variable and the loop variable are distinct, leaving getPeerStates and toSyncPeerConnectionState behavior unchanged.
1092-1108: Stale lane presence for superseded device until TTL.
peer.metadatais cleared beforews.close(), but theclosehandler at L812-819 readspeer.metadata?.deviceIdto driveremoveAllPresenceForDevice(...). Because metadata is alreadynull, the cleanup short-circuits and any"remote"lane-presence entries for this device linger untilLANE_PRESENCE_TTL_MS(60s) — unless the freshly authenticated peer happens to re-announce the same lanes. Since the new peer typically only announces lanes the user is currently viewing, a previously-open lane can show stale presence on other devices for up to a minute.Capturing the device id and cleaning up explicitly here keeps presence consistent with the supersede.
♻️ Suggested cleanup before clearing metadata
function closeExistingPeersForDevice(deviceId: string, currentPeer: PeerState): void { const normalized = toOptionalString(deviceId); if (!normalized) return; + let presenceChanged = false; for (const peer of peers) { if (peer === currentPeer) continue; if (peer.metadata?.deviceId !== normalized && peer.pairedDeviceId !== normalized) continue; + const supersededDeviceId = peer.metadata?.deviceId ?? peer.pairedDeviceId ?? normalized; + if (removeAllPresenceForDevice(supersededDeviceId, "remote")) { + presenceChanged = true; + } peer.authenticated = false; peer.metadata = null; peer.authKind = null; peer.pairedDeviceId = null; try { peer.ws.close(4000, "Superseded by a newer connection for this device"); } catch { // ignore close failures } } + if (presenceChanged) { + args.onStateChanged?.(); + broadcastBrainStatus(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncHostService.ts` around lines 1092 - 1108, In closeExistingPeersForDevice, capture the device id from peer.metadata (e.g., const staleDeviceId = peer.metadata?.deviceId) before you null out peer.metadata, then call removeAllPresenceForDevice(staleDeviceId) (or equivalent cleanup) for each superseded peer prior to or immediately after calling peer.ws.close; this ensures the close handler (which reads peer.metadata?.deviceId) doesn't short-circuit and remote lane-presence for the superseded device is removed immediately rather than waiting for TTL. Reference: closeExistingPeersForDevice, peer.metadata, peer.ws.close, and removeAllPresenceForDevice.apps/ios/ADE/Services/KeychainService.swift (1)
9-14: Optional: trim once.
hostKey.trimmingCharacters(in: .whitespacesAndNewlines)runs twice for non-empty inputs. Tiny win, but tightens the intent.♻️ Suggested tweak
private func tokenAccount(for hostKey: String?) -> String { - guard let hostKey, !hostKey.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { - return tokenAccount - } - return "\(tokenAccount):\(hostKey.trimmingCharacters(in: .whitespacesAndNewlines))" + let trimmed = hostKey?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" + guard !trimmed.isEmpty else { return tokenAccount } + return "\(tokenAccount):\(trimmed)" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Services/KeychainService.swift` around lines 9 - 14, In tokenAccount(for:) avoid trimming hostKey twice by binding a single trimmed value: first guard-let hostKey to non-nil, then create trimmedHostKey = hostKey.trimmingCharacters(in: .whitespacesAndNewlines) and check trimmedHostKey.isEmpty; return tokenAccount if empty, otherwise return "\(tokenAccount):\(trimmedHostKey)"; this preserves current behavior but performs the trim only once.
🤖 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/ios/ADE/Services/SyncService.swift`:
- Around line 1362-1371: When saving a profile, ensure any previous storage key
that maps to the same HostConnectionProfile is removed before inserting the new
key: use profileStorageKey(_:) to compute the new key and scan the existing
profilesKey dictionary (or list) for any entries whose underlying
HostConnectionProfile matches the same host (e.g., same
hostIdentity/lastHostDeviceId/hostName+port or lastSuccessfulAddress+port) and
delete that old key first, then write the new key; apply the same fix in the
other save path that mirrors saveProfile(_: ) (the duplicate block referenced in
the review) so disconnect(clearCredentials:) will not be left with stale
aliases.
- Around line 1482-1486: The guard that selects a profile and checks
tokenForProfile(profile) should not set the global connectionState to .error
because it doesn't tear down any existing socket; instead remove or avoid
mutating connectionState in this branch and only set lastError to the message
(or explicitly call the disconnect/teardown routine before setting .error if you
intend to mark the whole service down). Locate the guard around
candidates.sorted(by: { $0.updatedAt > $1.updatedAt }).first /
tokenForProfile(profile), remove the line setting connectionState = .error, and
either leave lastError = "This saved computer..." and return, or call the sync
teardown/disconnect function first and then set connectionState = .error if a
full disconnect was performed.
In `@configuration/overview.mdx`:
- Line 17: Update the ambiguous comment for "packs/" to match the actual
contents and the wording used in lanes/packs.mdx: replace "Compatibility
snapshots and audit artifacts" with a concise, consistent description such as
"Lane Pack briefings and audit snapshots" and mention the storage layout
reference (.ade/artifacts/packs/lanes/<lane-id>/current.md) so the packs/ entry
and lanes/packs.mdx use the same terminology and accurately reflect what lives
under packs/.
In `@configuration/settings.mdx`:
- Around line 95-105: The Workspace intro mentions "workspace-level skill files"
but the settings table (containing rows for "Project name", "Project
description", "Default working directory", and "baseRef") doesn’t document them;
either add a new table row describing "Workspace skill files" (location,
naming/convention, and how they’re configured/loaded) or edit the intro text to
remove/soften the reference to skill files so it matches the table
content—update the "Workspace" section and the table entries (e.g., add a
"Workspace skill files" row) to keep the docs consistent.
- Around line 367-369: Remove the stray closing tags at the end of the document:
delete the trailing "</content>" and "</invoke>" lines that follow the CardGroup
closing tag (i.e., after the </CardGroup> element) so the MDX/ Mintlify file
ends cleanly with valid MDX content; ensure no other leftover scaffolding tags
remain and run the doc build to confirm rendering succeeds.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/app/AppShell.tsx`:
- Around line 179-183: Remove the dead AI banner state and UI: delete the unused
state declarations aiFailure and aiMockProvider (and their types AiBannerState
and the inline createdAt shape) and remove any calls to their setters
setAiFailure and setAiMockProvider, then remove the JSX that renders the AI
banners tied to those states inside the AppShell component (the unreachable
banner blocks). Keep aiStatus and related AI settings logic if still used.
Ensure imports/types only referenced by the removed code are also cleaned up.
In `@configuration/settings.mdx`:
- Line 314: Remove the stale recommendation "**Auto-refresh events: On PR create
+ On mission start**" from the Solo Developer recommended-settings section (the
bullet text shown in the diff) because the context-doc auto-refresh feature was
removed; either delete this bullet entirely or replace it with a
currently-supported setting (e.g., another existing Workspace toggle) and ensure
the surrounding markdown in settings.mdx updates accordingly so the
recommendations no longer reference the deleted feature.
In `@lanes/packs.mdx`:
- Line 124: The sentence in lanes/packs.mdx referencing "Settings > Context >
Pack History Retention" is stale; open lanes/packs.mdx, locate the paragraph
that contains "Settings > Context > Pack History Retention" (the Pack files
description) and either remove the broken UI pointer or replace it with the
correct/current location or wording (e.g., a generic note that history retention
is configurable in Settings or link to the updated configuration docs). Update
the text so it no longer points to the nonexistent "Settings > Context" section.
- Around line 1-9: The doc currently contradicts itself: the frontmatter
description says Lane Packs are "auto-generated, versioned context bundles" but
the opening sentence calls them a static "structured lane briefing," while
several sections (Pack Freshness, Context Injection, automatic regeneration on
commit, green/amber/red indicators, Project/Feature/Conflict packs, history
retention) still describe auto-generation/injection behavior that this PR
removes. Decide which model to keep and reconcile the content: either (A)
restore the auto-generation narrative by reverting the static wording in the
opening paragraph and ensuring the frontmatter `description` and sections like
"Pack Freshness", "Context Injection", and mentions of automatic regeneration
and freshness indicators remain and align with terms "Lane Pack", "Pack
Freshness", and "Context Injection"; or (B) remove/strip all stale
auto-generation/injection sections (Pack Freshness, Context Injection, automatic
regeneration on commit, freshness indicators, Project/Feature/Conflict packs,
history retention) and update the frontmatter `description` and the opening
paragraph to clearly define a static "structured lane briefing" and any new
behavior. Apply the chosen change consistently across the document so the
frontmatter, the opening "What is a Lane Pack?" paragraph, and all headings use
the same model.
---
Nitpick comments:
In `@apps/desktop/src/main/services/orchestrator/promptInspector.ts`:
- Around line 210-213: The two guidance sentences added via sections.push in
promptInspector.ts are split into separate array entries (causing a blank
paragraph), which is inconsistent with the worker prompt where the same guidance
is joined with a single newline; update the promptInspector.ts call to combine
those two strings into a single sections.push entry using "\n" between them so
its formatting matches the sections.push usage in baseOrchestratorAdapter
(locate the sections.push call in promptInspector.ts and mirror the
single-string-with-"\n" pattern used in baseOrchestratorAdapter).
In `@apps/desktop/src/main/services/sync/syncHostService.ts`:
- Around line 2160-2172: The loop in getPeerStates uses the identifier peer
twice (once for PeerState in the map/filter chain that calls
toSyncPeerConnectionState, and again for SyncPeerConnectionState in the
for...of), causing variable shadowing that reduces readability; rename the inner
lambda parameter (e.g., rawPeer or p) in the [...peers].map(...)/.filter(...)
chain so the mapped variable and the loop variable are distinct, leaving
getPeerStates and toSyncPeerConnectionState behavior unchanged.
- Around line 1092-1108: In closeExistingPeersForDevice, capture the device id
from peer.metadata (e.g., const staleDeviceId = peer.metadata?.deviceId) before
you null out peer.metadata, then call removeAllPresenceForDevice(staleDeviceId)
(or equivalent cleanup) for each superseded peer prior to or immediately after
calling peer.ws.close; this ensures the close handler (which reads
peer.metadata?.deviceId) doesn't short-circuit and remote lane-presence for the
superseded device is removed immediately rather than waiting for TTL. Reference:
closeExistingPeersForDevice, peer.metadata, peer.ws.close, and
removeAllPresenceForDevice.
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Line 1697: Update the stale inline comment in the AgentChatComposer component:
remove the word "context" from the right-side controls comment and update it to
accurately list the rendered controls (e.g., "Right: attachment, commands,
proof, send") so the inline header near the comment in AgentChatComposer.tsx
matches the actual UI elements.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.test.ts`:
- Around line 77-85: The test name is outdated because project-doc prepending
was removed; update the spec title to match current behavior (e.g., rename the
it(...) description to "preserves the default attachment review request" or
"preserves the default attachment review request when no project docs are
prepended") so it accurately describes the assertion around
buildParallelLaunchPrompt returning displayText and sendText equal to "Please
review the attached files."; adjust only the test description string near the
assertions referencing buildParallelLaunchPrompt, displayText, and sendText.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 2934-3006: finalText is an unused alias of text in AgentChatPane;
remove the let finalText = text declaration and replace all uses of finalText
with text (notably inside optimisticEnvelope, the window.ade.agentChat.steer
call, and the window.ade.agentChat.send call), and remove any now-unused
variable declarations so AgentChatPane no longer contains the redundant
finalText symbol.
- Around line 702-716: buildParallelLaunchPrompt currently returns an object
with sendText and displayText that are always identical; simplify it to return a
single string (e.g., return the chosen text or empty string) and update all call
sites that destructure { sendText, displayText } (search for
buildParallelLaunchPrompt(...) usages) to accept a single string and map it to
the appropriate parameters (e.g., sendText/displayText or directly pass to
agentChat.send). Keep the fallback logic (trim input, use
DEFAULT_PARALLEL_ATTACHMENT_REQUEST when attachmentCount > 0) inside
buildParallelLaunchPrompt to preserve behavior.
In `@apps/desktop/src/shared/adeCliGuidance.ts`:
- Around line 1-12: The two exports ADE_CLI_AGENT_GUIDANCE and
ADE_CLI_INLINE_GUIDANCE duplicate the same guidance and risk drift; refactor by
creating a single source-of-truth array (e.g., ADE_CLI_GUIDANCE_LINES)
containing the guidance lines and derive ADE_CLI_AGENT_GUIDANCE via
ADE_CLI_GUIDANCE_LINES.join("\n") and ADE_CLI_INLINE_GUIDANCE by joining the
same array into a single inline sentence string (e.g.,
ADE_CLI_GUIDANCE_LINES.join(" ") with any minimal whitespace normalization),
then replace the current duplicated literals with those derived values so all
edits are made in one place.
In `@apps/ios/ADE/Services/KeychainService.swift`:
- Around line 9-14: In tokenAccount(for:) avoid trimming hostKey twice by
binding a single trimmed value: first guard-let hostKey to non-nil, then create
trimmedHostKey = hostKey.trimmingCharacters(in: .whitespacesAndNewlines) and
check trimmedHostKey.isEmpty; return tokenAccount if empty, otherwise return
"\(tokenAccount):\(trimmedHostKey)"; this preserves current behavior but
performs the trim only once.
In `@chat/capabilities.mdx`:
- Around line 30-31: The Card with title "Repo inspection" currently uses
icon="database" which is semantically mismatched; update the icon prop on that
Card (Card title="Repo inspection" icon="database") to a file/search-oriented
Font Awesome icon such as "magnifying-glass", "folder-tree", or
"file-magnifying-glass" so the icon better reflects inspecting repository docs,
source files, file maps, and activity.
🪄 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: c2530390-1061-4f98-9dda-e101e9a98685
⛔ Files ignored due to path filters (14)
docs.jsonis excluded by!docs.jsondocs/ARCHITECTURE.mdis excluded by!docs/**docs/PRD.mdis excluded by!docs/**docs/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/context-packs/README.mdis excluded by!docs/**docs/features/context-packs/freshness-and-delivery.mdis excluded by!docs/**docs/features/files-and-editor/README.mdis excluded by!docs/**docs/features/files-and-editor/file-watcher-and-trust.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/configuration-schema.mdis excluded by!docs/**docs/features/onboarding-and-settings/first-run.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**key-concepts.mdxis excluded by!*.mdx
📒 Files selected for processing (57)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/bootstrap.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/ai/tools/systemPrompt.test.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/context/contextDocBuilder.test.tsapps/desktop/src/main/services/context/contextDocBuilder.tsapps/desktop/src/main/services/context/contextDocService.test.tsapps/desktop/src/main/services/context/contextDocService.tsapps/desktop/src/main/services/cto/ctoStateService.tsapps/desktop/src/main/services/cto/workerAdapterRuntimeService.test.tsapps/desktop/src/main/services/files/fileService.test.tsapps/desktop/src/main/services/files/fileWatcherService.test.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.tsapps/desktop/src/main/services/orchestrator/baseOrchestratorAdapter.tsapps/desktop/src/main/services/orchestrator/missionLifecycle.tsapps/desktop/src/main/services/orchestrator/promptInspector.tsapps/desktop/src/main/services/orchestrator/stepPolicyResolver.tsapps/desktop/src/main/services/sync/syncHostService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/context/contextShared.test.tsapps/desktop/src/renderer/components/context/contextShared.tsapps/desktop/src/renderer/components/files/FilesPage.test.tsxapps/desktop/src/renderer/components/onboarding/ProjectSetupPage.tsxapps/desktop/src/renderer/components/settings/ContextSection.tsxapps/desktop/src/renderer/components/settings/GeneralSection.tsxapps/desktop/src/renderer/components/settings/WorkspaceSettingsSection.tsxapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/contextContract.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/config.tsapps/desktop/src/shared/types/packs.tsapps/ios/ADE/Services/KeychainService.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Settings/SettingsPairingSection.swiftapps/web/src/components/editorial/IndexPage.tsxchat/capabilities.mdxchat/context.mdxconfiguration/overview.mdxconfiguration/settings.mdxcontext-packs/freshness.mdxcontext-packs/overview.mdxcontext-packs/structure.mdxgetting-started/open-project.mdxgetting-started/project-setup.mdxlanes/packs.mdx
💤 Files with no reviewable changes (23)
- apps/desktop/src/renderer/components/settings/WorkspaceSettingsSection.tsx
- context-packs/freshness.mdx
- apps/web/src/components/editorial/IndexPage.tsx
- apps/desktop/src/main/services/orchestrator/missionLifecycle.ts
- context-packs/overview.mdx
- apps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsx
- apps/desktop/src/shared/types/config.ts
- apps/desktop/src/shared/contextContract.ts
- apps/desktop/src/renderer/components/context/contextShared.test.ts
- context-packs/structure.mdx
- apps/desktop/src/main/services/context/contextDocBuilder.test.ts
- apps/desktop/src/shared/ipc.ts
- apps/desktop/src/preload/preload.ts
- apps/ade-cli/src/bootstrap.ts
- apps/desktop/src/main/services/context/contextDocService.ts
- apps/desktop/src/renderer/components/settings/ContextSection.tsx
- apps/desktop/src/renderer/components/context/contextShared.ts
- apps/desktop/src/shared/types/packs.ts
- apps/desktop/src/renderer/state/appStore.ts
- apps/desktop/src/preload/global.d.ts
- apps/desktop/src/main/main.ts
- apps/desktop/src/main/services/context/contextDocBuilder.ts
- apps/desktop/src/main/services/context/contextDocService.test.ts
| private func profileStorageKey(_ profile: HostConnectionProfile) -> String? { | ||
| [ | ||
| profile.hostIdentity, | ||
| profile.lastHostDeviceId, | ||
| profile.hostName.map { "\($0):\(profile.port)" }, | ||
| profile.lastSuccessfulAddress.map { "\($0):\(profile.port)" }, | ||
| ] | ||
| .compactMap { $0?.trimmingCharacters(in: .whitespacesAndNewlines) } | ||
| .first { !$0.isEmpty } | ||
| } |
There was a problem hiding this comment.
Clean up the old saved-profile alias when the storage key changes.
profileStorageKey(_:) can legitimately change over time — e.g. the first save may use lastSuccessfulAddress:port, then hello fills hostIdentity and the same host gets a new key. saveProfile(_:) only writes the new key, so the old entry survives in profilesKey. That can duplicate saved hosts and leave disconnect(clearCredentials:) clearing only one alias of the same host. Please remove any previous entry that refers to the same host before inserting the new key.
Also applies to: 3533-3541
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Services/SyncService.swift` around lines 1362 - 1371, When
saving a profile, ensure any previous storage key that maps to the same
HostConnectionProfile is removed before inserting the new key: use
profileStorageKey(_:) to compute the new key and scan the existing profilesKey
dictionary (or list) for any entries whose underlying HostConnectionProfile
matches the same host (e.g., same hostIdentity/lastHostDeviceId/hostName+port or
lastSuccessfulAddress+port) and delete that old key first, then write the new
key; apply the same fix in the other save path that mirrors saveProfile(_: )
(the duplicate block referenced in the review) so disconnect(clearCredentials:)
will not be left with stale aliases.
| guard let profile = candidates.sorted(by: { $0.updatedAt > $1.updatedAt }).first, | ||
| tokenForProfile(profile) != nil else { | ||
| lastError = "This saved computer no longer has pairing credentials. Pair again from Settings." | ||
| connectionState = .error | ||
| return |
There was a problem hiding this comment.
Don't mark the whole sync service as .error when this reconnect can't even start.
This branch does not tear down the current socket. If the user is still connected to another host, setting connectionState = .error makes the app think sync is down and canSendLiveRequests() will start returning false even though the live connection is still up. Surface the failure via lastError (or disconnect first) instead of mutating the global connection state here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Services/SyncService.swift` around lines 1482 - 1486, The guard
that selects a profile and checks tokenForProfile(profile) should not set the
global connectionState to .error because it doesn't tear down any existing
socket; instead remove or avoid mutating connectionState in this branch and only
set lastError to the message (or explicitly call the disconnect/teardown routine
before setting .error if you intend to mark the whole service down). Locate the
guard around candidates.sorted(by: { $0.updatedAt > $1.updatedAt }).first /
tokenForProfile(profile), remove the line setting connectionState = .error, and
either leave lastError = "This saved computer..." and return, or call the sync
teardown/disconnect function first and then set connectionState = .error if a
full disconnect was performed.
| worktrees/ # Git worktrees for each lane (auto-managed by ADE) | ||
| artifacts/ | ||
| packs/ # Context pack snapshots | ||
| packs/ # Compatibility snapshots and audit artifacts |
There was a problem hiding this comment.
Comment for packs/ is vague and inconsistent with lanes/packs.mdx.
"Compatibility snapshots and audit artifacts" is ambiguous (compatibility with what?), and it conflicts with lanes/packs.mdx (still in this PR), which documents .ade/artifacts/packs/lanes/<lane-id>/current.md as the storage layout for Lane Pack briefings. Consider aligning the comment with what actually lives there (e.g., "Lane Pack briefings and audit snapshots") so docs stay consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@configuration/overview.mdx` at line 17, Update the ambiguous comment for
"packs/" to match the actual contents and the wording used in lanes/packs.mdx:
replace "Compatibility snapshots and audit artifacts" with a concise, consistent
description such as "Lane Pack briefings and audit snapshots" and mention the
storage layout reference (.ade/artifacts/packs/lanes/<lane-id>/current.md) so
the packs/ entry and lanes/packs.mdx use the same terminology and accurately
reflect what lives under packs/.
| ## Workspace | ||
|
|
||
| Project identity, file paths, and how ADE generates and refreshes context docs. This section combines what older builds split across a separate **Project** tab and **Context** tab. | ||
| Project identity, file paths, and workspace-level skill files. | ||
|
|
||
| | Setting | Description | Default | | ||
| |---------|-------------|---------| | ||
| | Project name | Display name shown in ADE's title bar | Derived from repository folder name | | ||
| | Project description | Short description shown on Project Home | Empty | | ||
| | Default working directory | Root directory for terminal sessions | Repository root | | ||
| | baseRef | Primary integration branch for PRs | `main` | | ||
| | Context doc model | AI model used to regenerate project context docs | `claude-sonnet-4-6` | | ||
| | Context doc reasoning effort | Depth of analysis during generation | Default (model decides) | | ||
| | Auto-refresh events | Which project events trigger automatic doc regeneration | On PR create, On mission start | | ||
|
|
||
| ### Auto-Refresh Events | ||
|
|
||
| Toggle individual events that trigger a context doc regeneration: | ||
|
|
||
| - **On session end** — regenerate when a terminal/agent session ends | ||
| - **On commit** — regenerate when a commit is created | ||
| - **On PR create** — regenerate when a pull request is created or updated | ||
| - **On PR land** — regenerate when a pull request is merged | ||
| - **On mission start** — regenerate when a mission launches | ||
| - **On mission end** — regenerate when a mission completes | ||
| - **On lane create** — regenerate when a new lane is created | ||
|
|
||
| Preferences are saved automatically and persist across sessions. | ||
|
|
There was a problem hiding this comment.
Workspace intro mentions “skill files” but the table doesn’t document them.
Lines 20 and 97 advertise “workspace-level skill files,” yet the Workspace settings table only lists project name/description/working directory/baseRef. Either add a row describing where skill files live and how they’re configured, or align the intro language with what the section actually documents to avoid leaving readers hunting for a setting that isn’t there.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@configuration/settings.mdx` around lines 95 - 105, The Workspace intro
mentions "workspace-level skill files" but the settings table (containing rows
for "Project name", "Project description", "Default working directory", and
"baseRef") doesn’t document them; either add a new table row describing
"Workspace skill files" (location, naming/convention, and how they’re
configured/loaded) or edit the intro text to remove/soften the reference to
skill files so it matches the table content—update the "Workspace" section and
the table entries (e.g., add a "Workspace skill files" row) to keep the docs
consistent.
| </CardGroup> | ||
| </content> | ||
| </invoke> No newline at end of file | ||
| </invoke> |
There was a problem hiding this comment.
Stray </invoke> artifact at end of file.
The trailing </content> and </invoke> lines appear to be leftover scaffolding from an LLM/tool write rather than valid Mintlify/MDX content. This will either render verbatim or break the doc build. Drop these lines.
Proposed fix
<Card title="Configuration Overview" icon="gear" href="/configuration/overview">
Review the .ade/ directory structure and the three config file layers.
</Card>
</CardGroup>
-</content>
-</invoke>
-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@configuration/settings.mdx` around lines 367 - 369, Remove the stray closing
tags at the end of the document: delete the trailing "</content>" and
"</invoke>" lines that follow the CardGroup closing tag (i.e., after the
</CardGroup> element) so the MDX/ Mintlify file ends cleanly with valid MDX
content; ensure no other leftover scaffolding tags remain and run the doc build
to confirm rendering succeeds.
…follow-ups Removes contextDocBuilder/Service, contextShared (renderer), ContextSection, context-packs/* docs, and dependent-file cleanups. Updates orchestrator, chat service, prompts, settings UI, and feature docs to drop references. Also picks up unrelated iOS keychain/sync polish and apps/web index entry removal that landed on this branch. # Conflicts: # apps/desktop/src/main/services/chat/agentChatService.ts # apps/desktop/src/main/services/cto/workerAdapterRuntimeService.test.ts # apps/ios/ADE/Services/KeychainService.swift # apps/ios/ADE/Services/SyncService.swift # apps/ios/ADE/Views/Settings/SettingsPairingSection.swift
Adds encodedEnvelopeBytes() helper alongside sendAndWait, peer-handshake test coverage, sync envelope/payload type extensions, and matching iOS RemoteModels + SyncService changes. Bundles in-flight sync work the user finished while shipLane was waiting.
arul28
force-pushed
the
ade/context-doc-generation-141e1dd5
branch
from
b214659 to
52711f6
Compare
|
@copilot review but do not make fixes |
Here's the review of the new commit (52711f6). No fixes applied per your request. Commit 4 — Sync envelope size helper, peer-handshake tests, iOS sync metadata (
|
…-desktop shards 1, 8)
|
@copilot review but do not make fixes |
Here's the review of the updated PR (commit 5 + two new Greptile P2s). No fixes applied per your request. Commit 5 — Sync test assertions to rebased prompt strings (
|
There was a problem hiding this comment.
Actionable comments posted: 3
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/onboarding/ProjectSetupPage.tsx (1)
290-298:⚠️ Potential issue | 🟡 MinorLast-step "Finish" gate is now indirectly tied to git via Linear.
With the context step removed,
isLastStepis nowlinear, so the existing(isLastStep && gitInstalled !== true)guard means the Finish button is disabled on the Linear page until git is verified. In the linear forward flow this is fine (you can't get pasttoolswithout git), but the sidebar lets users jump straight to any step (line 205), so a user who clicks "Linear" first will see Finish silently disabled with no inline explanation on that page.Not a regression of this PR — just calling it out so you can decide whether to (a) re-anchor the gate to the
toolsstep only, or (b) surface the "git required" hint on the Linear step too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/onboarding/ProjectSetupPage.tsx` around lines 290 - 298, The Finish button's disable logic currently ties git verification to isLastStep (used for the "linear" step), causing the Finish button to be disabled on the Linear page when gitInstalled !== true; update the guard so the git check applies only when advancing from the tools step (i.e., change the condition that references isLastStep to instead check step === "tools" before gating on gitInstalled) and/or add a visible hint on the Linear page about the git requirement; locate the Button component and its props (disabled, onClick) and the handleNext function as entry points to implement this change and ensure the sidebar direct navigation no longer silently disables Finish on non-tools steps.
♻️ Duplicate comments (2)
apps/ios/ADE/Services/SyncService.swift (2)
1535-1539:⚠️ Potential issue | 🟠 MajorAvoid setting the whole service to
.errorwhen this reconnect cannot even start.Line 1538 mutates the global
connectionStatewithout tearing down any existing socket. If the phone is still connected to another host, this flips the entire sync service into an error state and suppresses live requests even though the transport is still healthy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Services/SyncService.swift` around lines 1535 - 1539, The code currently sets connectionState = .error when tokenForProfile(profile) is nil, which mutates the global sync service state without tearing down sockets; change this so that when tokenForProfile(profile) == nil you only set lastError (with the message) and return early without changing connectionState or touching active transports (i.e., remove the connectionState = .error assignment), or alternatively set a local reconnectError flag scoped to this reconnect attempt; locate the check around candidates.sorted(by: { $0.updatedAt > $1.updatedAt }).first and tokenForProfile(profile) and update that branch to avoid global state mutation and ensure no socket teardown is performed there.
3653-3661:⚠️ Potential issue | 🟠 MajorRemove the previous saved-host alias when the storage key changes.
This path only inserts the new
profileStorageKey(_:)entry. When a profile is first saved under an address/name key and later re-saved underhostIdentity, the old alias survives inprofilesKeyand its host-scoped token survives too. That leaves duplicate saved-host rows anddisconnect(clearCredentials:)only clears the newest alias.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Services/SyncService.swift` around lines 3653 - 3661, When saving a profile in saveProfile(_:), detect and remove any existing saved-host alias entries whose storage key differs from the new profileStorageKey(profile) (i.e. the old address/name key) so duplicates and stale host-scoped tokens are not left behind; do this by loading the current profiles via loadSavedProfilesRaw(), find any keys whose stored HostConnectionProfile represents the same underlying host/identity but != profileStorageKey(profile), remove those keys from the profiles dictionary, clear their host-scoped token (or call the existing token-clearing routine used for disconnect), then insert the new profiles[newKey] = profile, call saveSavedProfiles(profiles), and finally run migrateTokenIfNeeded(for: profile) if needed.
🧹 Nitpick comments (6)
apps/desktop/src/renderer/components/chat/AgentChatPane.test.ts (1)
77-85: Stale test description after removing project-docs behavior.The
it(...)title still reads "preserves the default attachment review request when project docs are prepended", but withincludeProjectDocsremoved frombuildParallelLaunchPrompt, this test no longer exercises any "project docs prepended" path — it just verifies the default text for empty input + attachments. The title will mislead future readers and grep results.✏️ Proposed rename
- it("preserves the default attachment review request when project docs are prepended", () => { + it("returns the default attachment review request for empty text with attachments", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatPane.test.ts` around lines 77 - 85, Update the test title in AgentChatPane.test.ts to reflect the current behavior of buildParallelLaunchPrompt (which no longer accepts includeProjectDocs); replace the misleading "preserves the default attachment review request when project docs are prepended" with a concise description like "returns the default attachment review request for empty text with attachments" so the test accurately documents that buildParallelLaunchPrompt({ text: "", attachmentCount: 2 }) yields the default displayText/sendText.apps/desktop/src/main/services/cto/ctoStateService.ts (1)
757-757: Rename + context-docs removal is clean; minor naming overlap to consider.The renames (
currentContextDocPath→currentContextPath, and the newcurrentContextBodylocal) preserve behavior —.ade/cto/CURRENT.mdis still written atomically with the same rendered content minus the now-removed “Project context docs” section. No functional concerns.Minor nit:
currentContextPathis somewhat ambiguous given the PR is simultaneously removing.ade/context/. Something likecurrentMemoryDocPathor keepingcurrentContextDocPathwould more clearly signal that this points to the CTOCURRENT.md, not anything under.ade/context/. Non-blocking.Also applies to: 1181-1187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/cto/ctoStateService.ts` at line 757, Rename the ambiguous variable currentContextPath to a clearer name (e.g., currentContextDocPath or currentMemoryDocPath) throughout ctoStateService.ts to indicate it points to the CTO CURRENT.md document; update all references where currentContextPath is declared and used (including the declaration near path.join(ctoDir, "CURRENT.md") and any reads/writes that use currentContextBody) to the chosen name so behavior remains unchanged but the intent is clearer.apps/desktop/src/shared/adeCliGuidance.ts (1)
1-12: Guidance text reads consistently with the rest of the agent contract.The new "only normal reason to skip ADE CLI…after those fallbacks" language matches the fallback-then-skip pattern already enforced elsewhere (e.g.,
prIssueResolverandbaseOrchestratorAdapter's "prefer external first / use ADE-local as fallback" rule). No functional concerns.Optional:
ADE_CLI_AGENT_GUIDANCEandADE_CLI_INLINE_GUIDANCEnow duplicate substantively the same instructions in two shapes (array join vs. single string). Easy to drift on future edits — consider deriving the inline form from the array (e.g.,ADE_CLI_AGENT_GUIDANCE.replace(/\n/g, " ")or a shared source list) if you touch this again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/shared/adeCliGuidance.ts` around lines 1 - 12, ADE_CLI_AGENT_GUIDANCE and ADE_CLI_INLINE_GUIDANCE duplicate the same guidance text in two forms; to prevent drift, export a single source (e.g., an array or a single string) and derive the other from it (for example keep ADE_CLI_AGENT_GUIDANCE as the array and set ADE_CLI_INLINE_GUIDANCE = ADE_CLI_AGENT_GUIDANCE.join(" ").replace(/\s+/g, " ") or vice versa), and update any references to use these symbols so future edits only change one source of truth.apps/desktop/src/main/services/sync/syncHostService.ts (3)
2321-2333: Per-device dedup via lexicographicconnectedAtcomparison.Comparing
peer.connectedAt > existing.connectedAtworks becausenowIso()emits ISO 8601 with fixed-width fields, so lexicographic order matches chronological order. Worth a brief inline comment so a future change to a different timestamp format doesn't silently break the "newest connection wins" invariant relied upon bysyncService.ts:670-680.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncHostService.ts` around lines 2321 - 2333, getPeerStates uses lexicographic comparison of peer.connectedAt to pick the newest connection per device; add a brief inline comment in getPeerStates (near the connectedAt comparison inside the loop) stating that connectedAt is ISO 8601 fixed-width (produced by nowIso()) so string lexicographic ordering equals chronological ordering and warning that changing the timestamp format would break the "newest connection wins" invariant relied on by syncService (see syncService usage around lines referenced). This makes the assumption explicit and helps future maintainers avoid silent regressions.
1234-1247:projectsForHelloestimate usesfeatures: {}— underestimates actualhello_oksize.The real
hello_okpayload built at lines 1875-1900 includesfeatures.commandRouting.actionswith ~170 descriptors plus other feature flags, easily adding 5–15 KB versus the empty-object stub used here. WithmaxProjectCatalogEnvelopeBytes = 768 KBand the WebSocketmaxPayload = 25 MB, going slightly over the soft limit is benign, but the check is a leaky abstraction: future feature additions silently erode the budget.♻️ Suggested tightening of the size estimate
function projectsForHello(projectCatalog: SyncProjectCatalogPayload): SyncMobileProjectSummary[] { - const payload = { - peer: readBrainMetadata(), - brain: readBrainMetadata(), - serverDbVersion: args.db.sync.getDbVersion(), - heartbeatIntervalMs, - pollIntervalMs, - projects: projectCatalog.projects, - features: {}, - }; - return encodedEnvelopeBytes("hello_ok", payload) <= maxProjectCatalogEnvelopeBytes + // Cheaper, more conservative: bound by encoded project_catalog size alone. + return encodedEnvelopeBytes("project_catalog", projectCatalog) <= maxProjectCatalogEnvelopeBytes ? projectCatalog.projects : []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncHostService.ts` around lines 1234 - 1247, projectsForHello currently estimates envelope size using features: {} which underestimates the real "hello_ok" payload (the real payload includes features.commandRouting.actions and other flags); update projectsForHello to use the same features structure used when constructing the real hello_ok (or call/reuse the function that builds those features) so encodedEnvelopeBytes("hello_ok", payload) uses a realistic features object (including commandRouting.actions descriptor count) before comparing to maxProjectCatalogEnvelopeBytes; reference encodedEnvelopeBytes, projectsForHello, the "hello_ok" payload, and features.commandRouting.actions when making this change.
1262-1271: Empty-chunk branch is unreachable.When
projectCatalog.projectsis empty,encodedEnvelopeBytes("project_catalog", ...)is well under the threshold, so the function returns via the single-message branch at line 1255. Theif (chunks.length === 0)block can never be entered. Safe to drop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncHostService.ts` around lines 1262 - 1271, Remove the unreachable empty-chunk branch: delete the if (chunks.length === 0) { ... } block that sends a "project_catalog_chunk" with an empty projects array, since when projectCatalog.projects is empty the earlier single-message branch (the call to encodedEnvelopeBytes("project_catalog", ...)) already returns. Update surrounding code to rely on the existing single-message handling (refer to encodedEnvelopeBytes("project_catalog", ...) and the existing send call) and remove references to SyncProjectCatalogChunkPayload in that removed branch.
🤖 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/sync/syncHostService.ts`:
- Around line 1148-1194: sendChangesetBatch can advertise a toDbVersion lower
than the caller's toDbVersion because flush() computes chunkToDbVersion from
Math.max over chunk rows; change flush() to use the caller-supplied toDbVersion
for the final chunk (i.e., compute chunkToDbVersion =
Math.max(...chunk.db_version) for intermediate flushes but when flushing the
last remaining chunk set chunkToDbVersion = toDbVersion) so the envelope always
advertises the true upper bound; also remove the unreachable trailing if
(changes.length === 0 && toDbVersion > fromDbVersion) branch, and keep
references to sendChangesetBatch, flush, chunkToDbVersion and
pumpChanges/exportChangesSince in mind when making the change.
In `@apps/ios/ADE/Services/KeychainService.swift`:
- Around line 9-14: The tokenAccount(for:) change from "." to ":" will orphan
existing host-scoped Keychain entries; update loadToken(hostKey:) to attempt
reading the new account (tokenAccount(for: hostKey)) and, on miss, try the
legacy account built as "\(tokenAccount).\(trimmedHost)" using the same trimming
logic, then if found call saveString(...) to save it under tokenAccount(for:
hostKey) and clearString(account: legacyAccount) to remove the old entry;
alternatively implement a one-time migration in an init/startup path that
enumerates/deletes legacy "connection-token.<host>" keys before using the new
format—references: tokenAccount(for:), loadToken(hostKey:),
clearToken(hostKey:), saveString(account:), loadString(account:).
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 1426-1431: The loadSavedProfilesRaw() function currently only
attempts to decode a [String: HostConnectionProfile] and returns [:] on failure,
which drops profiles saved in the old [HostConnectionProfile] format; update
loadSavedProfilesRaw() to first try decoding [String: HostConnectionProfile],
and if that fails attempt to decode [HostConnectionProfile] and convert it to a
dictionary (e.g., profiles.reduce(into: [:]) { $0[$1.id] = $1 } — or use the
appropriate unique property such as host/name if id is not present), returning
the converted dictionary; ensure the same migration logic is applied in the
other loading path referenced around the loadSavedProfiles() calls (the block
around lines 1434-1441) so old-array payloads are preserved.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/onboarding/ProjectSetupPage.tsx`:
- Around line 290-298: The Finish button's disable logic currently ties git
verification to isLastStep (used for the "linear" step), causing the Finish
button to be disabled on the Linear page when gitInstalled !== true; update the
guard so the git check applies only when advancing from the tools step (i.e.,
change the condition that references isLastStep to instead check step ===
"tools" before gating on gitInstalled) and/or add a visible hint on the Linear
page about the git requirement; locate the Button component and its props
(disabled, onClick) and the handleNext function as entry points to implement
this change and ensure the sidebar direct navigation no longer silently disables
Finish on non-tools steps.
---
Duplicate comments:
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 1535-1539: The code currently sets connectionState = .error when
tokenForProfile(profile) is nil, which mutates the global sync service state
without tearing down sockets; change this so that when tokenForProfile(profile)
== nil you only set lastError (with the message) and return early without
changing connectionState or touching active transports (i.e., remove the
connectionState = .error assignment), or alternatively set a local
reconnectError flag scoped to this reconnect attempt; locate the check around
candidates.sorted(by: { $0.updatedAt > $1.updatedAt }).first and
tokenForProfile(profile) and update that branch to avoid global state mutation
and ensure no socket teardown is performed there.
- Around line 3653-3661: When saving a profile in saveProfile(_:), detect and
remove any existing saved-host alias entries whose storage key differs from the
new profileStorageKey(profile) (i.e. the old address/name key) so duplicates and
stale host-scoped tokens are not left behind; do this by loading the current
profiles via loadSavedProfilesRaw(), find any keys whose stored
HostConnectionProfile represents the same underlying host/identity but !=
profileStorageKey(profile), remove those keys from the profiles dictionary,
clear their host-scoped token (or call the existing token-clearing routine used
for disconnect), then insert the new profiles[newKey] = profile, call
saveSavedProfiles(profiles), and finally run migrateTokenIfNeeded(for: profile)
if needed.
---
Nitpick comments:
In `@apps/desktop/src/main/services/cto/ctoStateService.ts`:
- Line 757: Rename the ambiguous variable currentContextPath to a clearer name
(e.g., currentContextDocPath or currentMemoryDocPath) throughout
ctoStateService.ts to indicate it points to the CTO CURRENT.md document; update
all references where currentContextPath is declared and used (including the
declaration near path.join(ctoDir, "CURRENT.md") and any reads/writes that use
currentContextBody) to the chosen name so behavior remains unchanged but the
intent is clearer.
In `@apps/desktop/src/main/services/sync/syncHostService.ts`:
- Around line 2321-2333: getPeerStates uses lexicographic comparison of
peer.connectedAt to pick the newest connection per device; add a brief inline
comment in getPeerStates (near the connectedAt comparison inside the loop)
stating that connectedAt is ISO 8601 fixed-width (produced by nowIso()) so
string lexicographic ordering equals chronological ordering and warning that
changing the timestamp format would break the "newest connection wins" invariant
relied on by syncService (see syncService usage around lines referenced). This
makes the assumption explicit and helps future maintainers avoid silent
regressions.
- Around line 1234-1247: projectsForHello currently estimates envelope size
using features: {} which underestimates the real "hello_ok" payload (the real
payload includes features.commandRouting.actions and other flags); update
projectsForHello to use the same features structure used when constructing the
real hello_ok (or call/reuse the function that builds those features) so
encodedEnvelopeBytes("hello_ok", payload) uses a realistic features object
(including commandRouting.actions descriptor count) before comparing to
maxProjectCatalogEnvelopeBytes; reference encodedEnvelopeBytes,
projectsForHello, the "hello_ok" payload, and features.commandRouting.actions
when making this change.
- Around line 1262-1271: Remove the unreachable empty-chunk branch: delete the
if (chunks.length === 0) { ... } block that sends a "project_catalog_chunk" with
an empty projects array, since when projectCatalog.projects is empty the earlier
single-message branch (the call to encodedEnvelopeBytes("project_catalog", ...))
already returns. Update surrounding code to rely on the existing single-message
handling (refer to encodedEnvelopeBytes("project_catalog", ...) and the existing
send call) and remove references to SyncProjectCatalogChunkPayload in that
removed branch.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.test.ts`:
- Around line 77-85: Update the test title in AgentChatPane.test.ts to reflect
the current behavior of buildParallelLaunchPrompt (which no longer accepts
includeProjectDocs); replace the misleading "preserves the default attachment
review request when project docs are prepended" with a concise description like
"returns the default attachment review request for empty text with attachments"
so the test accurately documents that buildParallelLaunchPrompt({ text: "",
attachmentCount: 2 }) yields the default displayText/sendText.
In `@apps/desktop/src/shared/adeCliGuidance.ts`:
- Around line 1-12: ADE_CLI_AGENT_GUIDANCE and ADE_CLI_INLINE_GUIDANCE duplicate
the same guidance text in two forms; to prevent drift, export a single source
(e.g., an array or a single string) and derive the other from it (for example
keep ADE_CLI_AGENT_GUIDANCE as the array and set ADE_CLI_INLINE_GUIDANCE =
ADE_CLI_AGENT_GUIDANCE.join(" ").replace(/\s+/g, " ") or vice versa), and update
any references to use these symbols so future edits only change one source of
truth.
🪄 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: 202b3b5e-41da-4fc2-8e68-0eb13d063606
⛔ Files ignored due to path filters (14)
docs.jsonis excluded by!docs.jsondocs/ARCHITECTURE.mdis excluded by!docs/**docs/PRD.mdis excluded by!docs/**docs/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/context-packs/README.mdis excluded by!docs/**docs/features/context-packs/freshness-and-delivery.mdis excluded by!docs/**docs/features/files-and-editor/README.mdis excluded by!docs/**docs/features/files-and-editor/file-watcher-and-trust.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/configuration-schema.mdis excluded by!docs/**docs/features/onboarding-and-settings/first-run.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**key-concepts.mdxis excluded by!*.mdx
📒 Files selected for processing (60)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/bootstrap.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/ai/tools/systemPrompt.test.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/context/contextDocBuilder.test.tsapps/desktop/src/main/services/context/contextDocBuilder.tsapps/desktop/src/main/services/context/contextDocService.test.tsapps/desktop/src/main/services/context/contextDocService.tsapps/desktop/src/main/services/cto/ctoStateService.tsapps/desktop/src/main/services/cto/ctoWorkerLifecycle.test.tsapps/desktop/src/main/services/files/fileService.test.tsapps/desktop/src/main/services/files/fileWatcherService.test.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.tsapps/desktop/src/main/services/orchestrator/baseOrchestratorAdapter.tsapps/desktop/src/main/services/orchestrator/missionLifecycle.tsapps/desktop/src/main/services/orchestrator/promptInspector.tsapps/desktop/src/main/services/orchestrator/stepPolicyResolver.tsapps/desktop/src/main/services/sync/syncHostService.test.tsapps/desktop/src/main/services/sync/syncHostService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/context/contextShared.test.tsapps/desktop/src/renderer/components/context/contextShared.tsapps/desktop/src/renderer/components/files/FilesPage.test.tsxapps/desktop/src/renderer/components/onboarding/ProjectSetupPage.tsxapps/desktop/src/renderer/components/settings/ContextSection.tsxapps/desktop/src/renderer/components/settings/GeneralSection.tsxapps/desktop/src/renderer/components/settings/WorkspaceSettingsSection.tsxapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/contextContract.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/config.tsapps/desktop/src/shared/types/packs.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/KeychainService.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Settings/SettingsPairingSection.swiftapps/web/src/components/editorial/IndexPage.tsxchat/capabilities.mdxchat/context.mdxconfiguration/overview.mdxconfiguration/settings.mdxcontext-packs/freshness.mdxcontext-packs/overview.mdxcontext-packs/structure.mdxgetting-started/open-project.mdxgetting-started/project-setup.mdxlanes/packs.mdx
💤 Files with no reviewable changes (23)
- apps/desktop/src/renderer/components/settings/WorkspaceSettingsSection.tsx
- apps/web/src/components/editorial/IndexPage.tsx
- apps/desktop/src/main/services/orchestrator/missionLifecycle.ts
- apps/desktop/src/shared/types/config.ts
- apps/desktop/src/shared/contextContract.ts
- apps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsx
- apps/ade-cli/src/bootstrap.ts
- context-packs/structure.mdx
- apps/desktop/src/main/services/context/contextDocService.test.ts
- context-packs/overview.mdx
- apps/desktop/src/shared/ipc.ts
- context-packs/freshness.mdx
- apps/desktop/src/preload/global.d.ts
- apps/desktop/src/renderer/components/settings/ContextSection.tsx
- apps/desktop/src/shared/types/packs.ts
- apps/desktop/src/main/services/context/contextDocService.ts
- apps/desktop/src/renderer/components/context/contextShared.test.ts
- apps/desktop/src/main/services/context/contextDocBuilder.test.ts
- apps/desktop/src/preload/preload.ts
- apps/desktop/src/renderer/components/context/contextShared.ts
- apps/desktop/src/main/main.ts
- apps/desktop/src/main/services/context/contextDocBuilder.ts
- apps/desktop/src/renderer/state/appStore.ts
✅ Files skipped from review due to trivial changes (13)
- configuration/overview.mdx
- apps/desktop/src/main/services/ai/tools/systemPrompt.test.ts
- apps/ios/ADE/Models/RemoteModels.swift
- apps/desktop/src/renderer/components/settings/GeneralSection.tsx
- apps/desktop/src/main/services/orchestrator/baseOrchestratorAdapter.ts
- apps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.ts
- getting-started/project-setup.mdx
- apps/desktop/src/main/services/cto/ctoWorkerLifecycle.test.ts
- lanes/packs.mdx
- apps/desktop/src/main/services/files/fileWatcherService.test.ts
- apps/desktop/src/main/services/files/fileService.test.ts
- getting-started/open-project.mdx
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- chat/context.mdx
- apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts
- apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx
- apps/ios/ADE/Views/Settings/SettingsPairingSection.swift
- chat/capabilities.mdx
- configuration/settings.mdx
- apps/desktop/src/renderer/components/files/FilesPage.test.tsx
| function sendChangesetBatch( | ||
| peer: PeerState, | ||
| reason: SyncChangesetBatchPayload["reason"], | ||
| fromDbVersion: number, | ||
| toDbVersion: number, | ||
| changes: CrsqlChangeRow[], | ||
| ): void { | ||
| let chunk: CrsqlChangeRow[] = []; | ||
| let chunkFromDbVersion = fromDbVersion; | ||
| let chunkBytes = 0; | ||
|
|
||
| const flush = (): void => { | ||
| if (chunk.length === 0) return; | ||
| const chunkToDbVersion = Math.max(...chunk.map((change) => Number(change.db_version ?? chunkFromDbVersion))); | ||
| send(peer.ws, "changeset_batch", { | ||
| reason, | ||
| fromDbVersion: chunkFromDbVersion, | ||
| toDbVersion: chunkToDbVersion, | ||
| changes: chunk, | ||
| }); | ||
| chunkFromDbVersion = chunkToDbVersion; | ||
| chunk = []; | ||
| chunkBytes = 0; | ||
| }; | ||
|
|
||
| for (const change of changes) { | ||
| const changeBytes = Buffer.byteLength(JSON.stringify(change), "utf8"); | ||
| if ( | ||
| chunk.length > 0 | ||
| && (chunk.length >= maxChangesetBatchRows || chunkBytes + changeBytes > maxChangesetBatchBytes) | ||
| ) { | ||
| flush(); | ||
| } | ||
| chunk.push(change); | ||
| chunkBytes += changeBytes; | ||
| } | ||
| flush(); | ||
|
|
||
| if (changes.length === 0 && toDbVersion > fromDbVersion) { | ||
| send(peer.ws, "changeset_batch", { | ||
| reason, | ||
| fromDbVersion, | ||
| toDbVersion, | ||
| changes: [], | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm how iOS tracks dbVersion from changeset_batch envelopes.
rg -nP -C5 'changeset_batch|toDbVersion|fromDbVersion' apps/ios/ADE
rg -nP -C3 'lastRemoteDbVersion|lastKnownServerDbVersion' apps/iosRepository: arul28/ADE
Length of output: 11983
🏁 Script executed:
#!/bin/bash
# Check pumpChanges function and how it filters changes before calling sendChangesetBatch
rg -A 20 'function pumpChanges|const pumpChanges' apps/desktop/src/main/services/sync/syncHostService.tsRepository: arul28/ADE
Length of output: 1113
🏁 Script executed:
#!/bin/bash
# Look at exportChangesSince and how it returns changes
rg -B5 -A15 'exportChangesSince' apps/desktop/src/main/services/sync/syncHostService.ts | head -60Repository: arul28/ADE
Length of output: 1164
🏁 Script executed:
#!/bin/bash
# Check line 1463 and surrounding context to see currentDbVersion advancement
sed -n '1450,1475p' apps/desktop/src/main/services/sync/syncHostService.tsRepository: arul28/ADE
Length of output: 1312
sendChangesetBatch final chunk advertises toDbVersion below the caller's actual upper bound.
When pumpChanges() filters exportChangesSince() results by site_id !== peer.metadata.siteId before calling sendChangesetBatch(), the filtered changes array may end with a row whose db_version is strictly less than the passed currentDbVersion parameter. Because flush() derives each chunk's toDbVersion from Math.max(...chunk.map(c => c.db_version)) rather than from the caller-supplied bound, the final chunk reports a version below the actual server state. Although the server then unconditionally advances peer.lastKnownServerDbVersion = currentDbVersion, the iOS client only sees batch.toDbVersion in the envelope and updates latestRemoteDbVersion accordingly—potentially recording a stale version that triggers redundant catchup on the next hello. iOS does also factor in result.dbVersion from applied changes, which provides partial mitigation, but edge cases where all applied changes carry lower versions could still result in stale tracking.
Also note: the trailing if (changes.length === 0 && toDbVersion > fromDbVersion) branch is unreachable from the only caller (pumpChanges() gates on changes.length > 0).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/sync/syncHostService.ts` around lines 1148 -
1194, sendChangesetBatch can advertise a toDbVersion lower than the caller's
toDbVersion because flush() computes chunkToDbVersion from Math.max over chunk
rows; change flush() to use the caller-supplied toDbVersion for the final chunk
(i.e., compute chunkToDbVersion = Math.max(...chunk.db_version) for intermediate
flushes but when flushing the last remaining chunk set chunkToDbVersion =
toDbVersion) so the envelope always advertises the true upper bound; also remove
the unreachable trailing if (changes.length === 0 && toDbVersion >
fromDbVersion) branch, and keep references to sendChangesetBatch, flush,
chunkToDbVersion and pumpChanges/exportChangesSince in mind when making the
change.
Recovers data orphaned by the rebase-resolved key-format changes: - KeychainService: read from legacy 'connection-token.<host>' format if the new ':' format misses, then re-save under the new format and delete the legacy entry. - SyncService: loadSavedProfilesRaw falls back to decoding the legacy [HostConnectionProfile] array, converts to the new dict format, and persists the new shape so future loads use the dict.
|
@copilot review but do not make fixes |
Here's the review of the latest Greptile pass (two new P2 findings). No fixes applied per your request. Greptile P2-A —
|
* fix(ios): restore profile dedup logic + remove dead reconnect path PR #209's rebase resolution kept the dict-based saved-profile API but dropped the recency-aware dedup that pre-existed on main. Restore it: - shouldPreferProfile picks the newer/Tailscale-preferring/has-last- successful-address profile when two map to the same storage key. - loadSavedProfilesRaw uses it during legacy [HostConnectionProfile] array migration so collisions don't fall to last-write-wins. - saveSavedProfiles runs a paranoia dedup pass before encoding. - reconnectToSavedHost (and its only helper profile(forSavedHost:)) removed; both were unreachable post-rebase, flagged by Greptile. * fix(ios): broader tailnet detection + cross-key profile dedup Addresses two CodeRabbit findings on #210: - profileHasTailnetRoute checks tailscaleAddress, lastSuccessfulAddress (via syncIsTailscaleRoute), and savedAddressCandidates so the shouldPreferProfile tie-break treats tailnet routes encoded in any field as equivalent to a populated tailscaleAddress. - deduplicatedProfiles now matches on hostIdentity, lastHostDeviceId, hostName:port, and lastSuccessfulAddress:port simultaneously, so profiles whose storage key evolved (e.g., older entry written before hostIdentity was learned) collapse with the newer stronger-keyed entry instead of leaving both in savedReconnectHosts.
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Refactor
ade prooffor inclusion in the proof drawer.Documentation
Greptile Summary
This PR removes the context doc generation system (PRD/Architecture doc builder,
contextDocService,contextDocBuilder, and related UI) in favour of agents inspecting the repo directly. It bundles two independent iOS sync improvements: chunked delivery for large mobile project catalogs, and a refactored multi-host token management layer inKeychainService/SyncService.Confidence Score: 4/5
Safe to merge; only P2 findings. Active-host reconnect works correctly; multi-host users on upgrade may need to re-pair non-active saved hosts.
All findings are P2. The token migration gap affects only multi-host users upgrading in place and the workaround is re-pairing. The dead-code path and missing chunk TTL are minor quality issues with no current runtime impact.
apps/ios/ADE/Services/SyncService.swift and apps/ios/ADE/Services/KeychainService.swift — legacy token key format migration.
Important Files Changed
Sequence Diagram
sequenceDiagram participant iOS as iOS SyncService participant Host as Desktop SyncHostService iOS->>Host: hello (deviceId, token) Host->>Host: closeExistingPeersForDevice(deviceId) Host-->>iOS: hello_ok (projects=[] if catalog too large) iOS->>Host: project_catalog_request Host->>Host: buildProjectCatalogPayload() alt catalog fits in envelope Host-->>iOS: project_catalog {projects} else catalog too large loop for each chunk Host-->>iOS: project_catalog_chunk {catalogId, index, total, done, projects} end iOS->>iOS: applyRemoteProjectCatalogChunk reassemble when count==total endComments Outside Diff (1)
apps/desktop/src/main/services/orchestrator/missionLifecycle.ts, line 307-315 (link).ade/context/PRD.ade.mdand.ade/context/ARCHITECTURE.ade.mdwere removed fromDOC_PRIORITY_REL_PATHSandstepPolicyResolver.tsas well. Users who previously generated these files will no longer have them surfaced as context for mission orchestration or step execution. If users have existing.ade/context/docs from the old feature, they will receive no context from them without manual migration guidance. Consider adding a migration note or scanning for these paths and logging a deprecation warning.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "ship: iter 3 — iOS upgrade migrations fo..." | Re-trigger Greptile