Deeplinks#326
Merged
Merged
Conversation
Snapshot of in-progress deeplinks work before pulling Tui Perf and Mac VM updates from main. Safe to soft-reset after the merge if you want these changes back in the working tree. - ade:// protocol handler + parser (apps/desktop/src/main/services/deeplinks) - iOS DeepLinkRouter extended for lane / repo / pr / linear-issue forms plus the cross-machine "Send to your Mac" path - syncRemoteCommandService.deeplinks.open for iOS -> desktop bounce - buildDeeplink + ensureAdeDeeplinkFooter shared utilities - LinearLaneCardService now publishes the cross-machine ADE deeplink - New renderer surfaces: ClipboardDeeplinkBanner, CrossRepoPrBanner, InboundDeeplinkModal - ade-deeplinks agent skill, OpenPage web handler, docs Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # apps/ade-cli/src/tuiClient/__tests__/adeApi.test.ts # apps/ade-cli/src/tuiClient/__tests__/appInput.test.ts # apps/ade-cli/src/tuiClient/app.tsx # apps/desktop/src/main/services/ipc/registerIpc.ts # apps/desktop/src/shared/adeCliGuidance.ts
- Code simplification across ade-cli, desktop main, and web (extract helpers, collapse nested ternaries, dead-branch removal) - Add multi-chat grid layout, hit-test registry, and add-chat mode to TUI client Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- ReviewPage: relax formatCompareTargetDescription param to the Pick subset it actually reads, fixing 4 TS2345s - SessionListPane test: align with refactored card (no inline tool label, lane name only in group header) - ChatAppearancePreview test: accept 22% or 28% accent mix (light-accent providers now use 22%) - ChatGitToolbar test: match new /lanes?laneId=... route and drop removed Run-menu test - iOS bootstrap SQL: regenerate to include lane_worktree_locks table Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 Walkthrough📝 Walkthrough✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Owner
Author
|
@copilot review but do not make fixes |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Contributor
Here's the review: 🐛 Bug: const BRANCH_BAD_RE = /(^|\/)\.\.($|\/)|[ -]/;
🔧
🔍 iOS self.kind = .repoBranch(owner: parts[0], repo: parts[1], branch: parts[3])For ℹ️ When the desktop isn't running, ℹ️ Dynamic dispatch in The |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
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 (3)
apps/ade-cli/src/tuiClient/components/Drawer.tsx (1)
173-193:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep add-mode UX consistent in
MiniDrawer.
addModeupdates full drawer labels/hints, but mini density still shows normal LANES/chats copy, which drops the “pick chat/add/cancel” guidance in the same flow.💡 Suggested patch
@@ <MiniDrawer width={width} borderColor={borderColor} + addMode={addMode} + emphasisColor={emphasisColor} lanes={laneRows} @@ function MiniDrawer({ width, borderColor, + addMode, + emphasisColor, @@ }: { width: number; borderColor: string; + addMode: boolean; + emphasisColor: string; @@ <Box paddingX={1}> - <Text bold color={theme.color.violet}> - LANES · {loading && lanes.length === 0 ? "…" : lanes.length} + <Text bold color={addMode ? emphasisColor : theme.color.violet}> + {addMode ? "PICK CHAT" : `LANES · ${loading && lanes.length === 0 ? "…" : lanes.length}`} </Text> </Box> @@ <Box paddingX={1} flexShrink={0}> <Text color={theme.color.t4} wrap="truncate"> - {!focused ? "\n" : mode === "chats" ? "↑↓ chats · Esc lanes" : "↑↓ lanes · ↵ open"} + {!focused + ? "\n" + : addMode + ? "↑↓ pick chat · ↵ add · esc cancel" + : mode === "chats" + ? "↑↓ chats · Esc lanes" + : "↑↓ lanes · ↵ open"} </Text> </Box>Also applies to: 640-643, 723-723
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/components/Drawer.tsx` around lines 173 - 193, The MiniDrawer isn't receiving the add-mode flag so it continues to render normal LANES/chats copy; update the Drawer render to pass the addMode prop into MiniDrawer (same way as FullDrawer/other densities) and ensure MiniDrawer's props/type (MiniDrawer component signature) accept and use addMode to switch labels/hints; also apply the same fix for the other indicated render sites (the other MiniDrawer usages around the file) so mini density shows the "pick chat / add / cancel" guidance when addMode is true.apps/desktop/src/main/services/ipc/runtimeBridge.ts (1)
685-700:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCategory changes won’t take effect for live subscriptions.
ensureRuntimeEventSubscriptiononly keys reuse onbindingKey(Line 269). After this change, whenrequest.categorychanges for the same binding, the old subscription is kept, so live pushed events can remain on the previous category.💡 Proposed fix
type RuntimeEventWindowSubscription = { bindingKey: string; + requestKey: string; cleanup: (() => void) | null; }; const ensureRuntimeEventSubscription = ( sender: WebContents, bindingKey: string, + requestKey: string, subscribe: RuntimeEventSubscribe, ): void => { const existing = runtimeEventSubscriptions.get(sender.id); - if (existing?.bindingKey === bindingKey) return; + if (existing?.requestKey === requestKey) return; cleanupRuntimeEventSubscription(sender.id); watchRuntimeEventSender(sender); - runtimeEventSubscriptions.set(sender.id, { bindingKey, cleanup: null }); + runtimeEventSubscriptions.set(sender.id, { bindingKey, requestKey, cleanup: null }); const onEnded = () => { const current = runtimeEventSubscriptions.get(sender.id); - if (current?.bindingKey === bindingKey) { + if (current?.requestKey === requestKey) { runtimeEventSubscriptions.delete(sender.id); } }; @@ if ( !current || - current.bindingKey !== bindingKey || + current.requestKey !== requestKey || sender.isDestroyed() ) { cleanup(); return; } @@ const current = runtimeEventSubscriptions.get(sender.id); - if (current?.bindingKey === bindingKey && !current.cleanup) { + if (current?.requestKey === requestKey && !current.cleanup) { runtimeEventSubscriptions.delete(sender.id); }ensureRuntimeEventSubscription( event.sender, binding.key, + `${binding.key}:${arg?.request?.category ?? "*"}`, (onEvent, onEnded) =>ensureRuntimeEventSubscription( event.sender, `remote:${target.id}:${projectId}`, + `remote:${target.id}:${projectId}:${arg?.request?.category ?? "*"}`, (onEvent, onEnded) =>Also applies to: 732-747
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/ipc/runtimeBridge.ts` around lines 685 - 700, ensureRuntimeEventSubscription currently deduplicates subscriptions only by binding.key, so changing request.category for the same binding reuses the old subscription and new category won't take effect; to fix, make the subscription key include the request's category (or entire request object) when calling ensureRuntimeEventSubscription from the local path branch — i.e., change the key argument from binding.key to a composite key (for example `${binding.key}:${arg?.request?.category}` or serialize the request) when invoking ensureRuntimeEventSubscription so localRuntimeConnectionPool.subscribeEventsForRoot is registered per-category and live events follow the updated category filter.apps/desktop/src/main/services/lanes/laneService.ts (1)
3829-4019:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope the worktree queue to the actual git mutation.
Wrapping the entire delete flow in
runGitWorktreeMutationmeansstopAll, watcher teardown,teardownEnv, the 250ms delay, and DB cleanup all hold the global worktree-mutation queue. If any of those steps is slow or hangs, unrelated lane create/import/adopt calls are blocked even though nogit worktreecommand is running. Keep the queue around thegit worktree remove/prunesection only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/lanes/laneService.ts` around lines 3829 - 4019, The outer await runGitWorktreeMutation(...) currently wraps the entire delete flow and must be removed so the global worktree-mutation queue only protects the actual git worktree removal; un-wrap the whole try/catch block from the outer call and instead keep the existing inner runStep("git_worktree_remove", ...) which already calls runGitWorktreeMutation(async () => { ... }); in its body (leave that inner runGitWorktreeMutation and the removeWorktreeDirectoryWithRecovery/ runGit calls intact). Concretely: delete the outer runGitWorktreeMutation invocation that begins before the try (the call using runGitWorktreeMutation and its closing paren after the catch), leaving all runStep(...) steps, the try/catch/finalize logic, and the inner runGitWorktreeMutation around the git worktree remove section unchanged.
🟡 Minor comments (15)
apps/ade-cli/src/commands/deeplinks.ts-95-104 (1)
95-104:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRequire
--linear-issuewhen using Linear flag mode.Line 95 currently allows
--branchwithout--linear-issue, which emits atype=linear-issueURL missingissue. That creates an ambiguous/invalid deeplink payload.Proposed fix
- if (linearIssue || branch) { + if (linearIssue || branch) { + if (!linearIssue) { + throw new CliDeeplinkUsageError("--linear-issue is required when using --branch"); + } // Build an https://ade.app/open URL with the hints Linear gave us. The🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/commands/deeplinks.ts` around lines 95 - 104, The code builds a "type=linear-issue" deeplink even when branch is provided without linearIssue, producing an invalid payload; update the validation in the deeplink handling so that when branch is set but linearIssue is missing (i.e., branch && !linearIssue) you surface a clear error/usage message and abort instead of calling openAndReport; modify the logic around linearIssue, branch, URLSearchParams and the call to openAndReport to check this case and fail fast (or require --linear-issue) so you never emit a type=linear-issue URL without an issue parameter.apps/ade-cli/src/commands/deeplinks.ts-235-240 (1)
235-240:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject repo slugs with more than one slash.
Line 236 accepts values like
a/b/cand silently truncates to ownera, repob. That can generate incorrect links.Proposed fix
function parseRepoSlug(repo: string): { repoOwner: string; repoName: string } { - const [repoOwner, repoName] = repo.split("/"); - if (!repoOwner || !repoName) { + const parts = repo.split("/"); + if (parts.length !== 2) { + throw new CliDeeplinkUsageError("Repo must be in 'owner/repo' form"); + } + const [repoOwner, repoName] = parts; + if (!repoOwner || !repoName) { throw new CliDeeplinkUsageError("Repo must be in 'owner/repo' form"); } return { repoOwner, repoName }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/commands/deeplinks.ts` around lines 235 - 240, The parseRepoSlug function currently splits on "/" and silently accepts strings like "a/b/c"; update parseRepoSlug to validate that repo.split("/") yields exactly two parts (length === 2) and throw CliDeeplinkUsageError("Repo must be in 'owner/repo' form") for any other case (zero, one, or more than two segments), ensuring it rejects slugs with more than one slash rather than truncating them.apps/ade-cli/src/tuiClient/app.tsx-7674-7677 (1)
7674-7677:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResolve the
Ctrl+Gshortcut collision.This branch consumes
Ctrl+Gbefore the existing external-editor path at Line 7969, so the prompt-editor shortcut becomes unreachable from chat.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/app.tsx` around lines 7674 - 7677, The Ctrl+G shortcut is being handled by the chat branch (isCtrlInput + startAddMode) before the external-editor handling, making the prompt-editor shortcut unreachable; change the logic so the external-editor path gets first dibs — either move the if (pane === "chat" && isCtrlInput(input, key, "g")) { startAddMode(); return; } block to after the external-editor handling, or add a guard so you only call startAddMode when the external-editor handler is not applicable (e.g., check the same condition/flag used by the external-editor path or call that handler and only call startAddMode if it returns false). Ensure you keep references to isCtrlInput and startAddMode while deferring to the external-editor handling.apps/ade-cli/src/tuiClient/components/GridMiniMap.tsx-13-16 (1)
13-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClamp focus to the rendered cell count, not raw
count.When
count > 6, the map renders 6 cells, but focus is clamped againstcount - 1. A high focused index can leave every rendered cell unfocused.💡 Suggested patch
export function gridMiniMapText(count: number, focusedIndex: number): string { - if (count <= 1) return "▣"; - return mapFor(count, Math.max(0, Math.min(focusedIndex, count - 1))); + const visibleCount = Math.max(1, Math.min(6, count)); + if (visibleCount <= 1) return "▣"; + return mapFor( + visibleCount, + Math.max(0, Math.min(focusedIndex, visibleCount - 1)), + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/components/GridMiniMap.tsx` around lines 13 - 16, gridMiniMapText clamps focus against the raw count which is wrong when the minimap renders a limited number of cells (6); compute the number of rendered cells first (e.g., const rendered = Math.min(count, 6) or use the same limit mapFor uses) and clamp focusedIndex against rendered - 1 instead of count - 1, then call mapFor(count, Math.max(0, Math.min(focusedIndex, rendered - 1))). Refer to gridMiniMapText and mapFor when making the change.apps/ade-cli/src/tuiClient/deeplinkRow.ts-65-76 (1)
65-76:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate PR number as a positive integer before building links.
On Line 66 and Line 75,
prNumberis not fully validated. Values like-1,1.5, orNaNcan still flow intobuildDeeplink(...)and produce invalid deeplinks.Proposed fix
export function buildDeeplinkForRow(row: DeeplinkRow): string | null { + const isValidPrNumber = (value: number): boolean => Number.isInteger(value) && value > 0; + if (row.kind === "lane") { if (!row.lane.id) return null; const target: DeeplinkTarget = { kind: "lane", laneId: row.lane.id }; return buildDeeplink(target, { form: "ade" }); } const pr = row.pr; if ("repoOwner" in pr) { - if (!pr.repoOwner || !pr.repoName || !pr.prNumber) return null; + if (!pr.repoOwner || !pr.repoName || !isValidPrNumber(pr.prNumber)) return null; return buildDeeplink( { kind: "pr", repoOwner: pr.repoOwner, repoName: pr.repoName, prNumber: pr.prNumber }, { form: "ade" }, ); } const parsed = parseGitHubPrUrl(pr.url); if (!parsed) return null; + const prNumber = pr.prNumber ?? parsed.prNumber; + if (!isValidPrNumber(prNumber)) return null; return buildDeeplink( - { kind: "pr", repoOwner: parsed.repoOwner, repoName: parsed.repoName, prNumber: pr.prNumber ?? parsed.prNumber }, + { kind: "pr", repoOwner: parsed.repoOwner, repoName: parsed.repoName, prNumber }, { form: "ade" }, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/deeplinkRow.ts` around lines 65 - 76, The PR number isn't validated as a positive integer before calling buildDeeplink, so values like -1, 1.5 or NaN can pass through; update the checks in the branch that handles "repoOwner" (where pr.prNumber is used) and in the parsed branch (where parsed.prNumber is used) to ensure the PR number is a positive integer (e.g., coerce to a number then require Number.isInteger(value) && value > 0) and return null if it fails, keeping the rest of the buildDeeplink calls unchanged; reference the symbols pr, parsed, pr.prNumber, parsed.prNumber, buildDeeplink and parseGitHubPrUrl to locate and modify the validation logic.apps/desktop/src/main/services/chat/agentChatService.ts-9676-9685 (1)
9676-9685:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
hook_progressis checked in the outer condition but not handled.The outer condition includes
hook_progress, but the innerif/else ifchain only handleshook_startedandhook_response. Ifhook_progressmessages enter this block, they'll fall through without the intended suppression.Proposed fix to suppress hook_progress alongside hook_started
if (hookMsg.subtype === "hook_started") { // Claude SDK hook start messages are high-frequency lifecycle noise. // Keep failures below, but do not persist successful hook bookkeeping // into the user-visible transcript. continue; + } else if (hookMsg.subtype === "hook_progress") { + // Also suppress hook progress messages. + continue; } else if (hookMsg.subtype === "hook_response") {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 9676 - 9685, The code checks for msg.type === "system" including subtype "hook_progress" but the inner branch only handles "hook_started" and "hook_response", so add handling for "hook_progress" in the same place as "hook_started" to suppress it; inside the block where hookMsg.subtype is inspected (the branch using hookMsg.subtype === "hook_started" / "hook_response"), add an else if (hookMsg.subtype === "hook_progress") { continue; } so progress messages are treated as lifecycle noise and not persisted to the transcript.apps/desktop/src/main/services/deeplinks/protocolHandler.ts-22-25 (1)
22-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle scheme matching case-insensitively in deeplink arg detection.
Line 24 only matches lowercase
ade://. Uppercase/mixed-case scheme inputs can be missed and never dispatched.Suggested fix
const ADE_OPEN_HTTPS_RE = /^https?:\/\/ade\.app\/open\b/i; +const ADE_SCHEME_RE = new RegExp(`^${ADE_DEEPLINK_SCHEME}://`, "i"); function isAdeDeeplinkArg(arg: unknown): arg is string { if (typeof arg !== "string") return false; - return arg.startsWith(`${ADE_DEEPLINK_SCHEME}://`) || ADE_OPEN_HTTPS_RE.test(arg); + return ADE_SCHEME_RE.test(arg) || ADE_OPEN_HTTPS_RE.test(arg); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/deeplinks/protocolHandler.ts` around lines 22 - 25, The scheme check in isAdeDeeplinkArg currently only matches lowercase ADE_DEEPLINK_SCHEME and can miss uppercase/mixed-case inputs; update the function to perform a case-insensitive scheme check by normalizing the arg (e.g., const s = String(arg).toLowerCase()) before testing startsWith(`${ADE_DEEPLINK_SCHEME}://`) or modify ADE_OPEN_HTTPS_RE to be case-insensitive (add the /i flag) so both the startsWith and regex checks accept uppercase/mixed-case schemes; keep the type guard name isAdeDeeplinkArg and use ADE_DEEPLINK_SCHEME and ADE_OPEN_HTTPS_RE to locate the change.apps/desktop/src/main/services/lanes/laneService.test.ts-3055-3057 (1)
3055-3057:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid fixed sleeps in these concurrency tests.
Line 3056 and Line 3109 rely on timing delays, which can make CI flaky. Prefer explicit latches/promises for “delete started” / “create reached worktree add” checkpoints.
Also applies to: 3108-3111
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/lanes/laneService.test.ts` around lines 3055 - 3057, Replace the fixed setTimeout-based waits with explicit synchronization promises: instrument the test to create a Promise (or latch) that is resolved from inside the code-path where the delete actually begins (e.g., in the stubbed/mocked implementation invoked by service.delete) and await that Promise before asserting service.hasRunningDelete(); do the same for the "create reached worktree add" checkpoint by resolving a Promise from the stub/mocked worktree.add (or the helper invoked by the create flow) instead of using setTimeout. Specifically, modify the test around service.delete and service.hasRunningDelete to await a "deleteStarted" promise resolved by the delete-start stub, and update the create/worktree add test to await a "createAtWorktreeAdd" promise resolved by the worktree add mock, removing both setTimeout usages.apps/desktop/src/main/services/prs/prService.ts-3563-3572 (1)
3563-3572:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve Linear reference in the final body-enrichment retry path.
If the earlier adopt-existing body PATCH fails, this pass only reapplies the ADE footer, so
closeLinearIssueOnMergeintent can be dropped even when this PATCH succeeds.💡 Proposed fix
- const currentBody = typeof pr?.body === "string" ? pr.body : prBody; - const enrichedBody = ensureAdeDeeplinkFooter(currentBody, { + const currentBody = typeof pr?.body === "string" ? pr.body : prBody; + const closeOnMerge = args.closeLinearIssueOnMerge === true; + const linearAdjustedCurrentBody = lane.linearIssue + ? ensureLinearPrReference( + currentBody, + lane.linearIssue, + closeOnMerge, + closeOnMerge ? { preserveExisting: false } : undefined, + ) + : currentBody; + const enrichedBody = ensureAdeDeeplinkFooter(linearAdjustedCurrentBody, { repoOwner: repo.owner, repoName: repo.name, branch: headBranch, prNumber, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/prs/prService.ts` around lines 3563 - 3572, The final retry that recomputes enrichedBody must preserve the Linear-close intent/reference from the previously adopted PR body so it isn't lost when the intermediate adopt-existing PATCH fails; update the currentBody selection logic used with ensureAdeDeeplinkFooter to prefer or merge in the original prBody's Linear metadata/intent (e.g., keep the closeLinearIssueOnMerge flag or Linear issue reference embedded in prBody) rather than using only pr?.body or a rebuilt prBody, so ensureAdeDeeplinkFooter receives a body that still contains the Linear reference.apps/desktop/src/main/services/runtime/processRegistryService.ts-92-104 (1)
92-104:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard heartbeat writes when the service is not started.
Line 92 and Line 107 can write
runtime_processeseven afterstop()/beforestart(), which can leave stale self rows and false-positive liveness.💡 Suggested fix
heartbeatTimer = setInterval(() => { + if (!started) return; try { writeOwnRow(null); } catch (error) { logger.warn("process_registry.heartbeat_failed", { pid, role, error: error instanceof Error ? error.message : String(error), }); } }, heartbeatIntervalMs); heartbeatTimer.unref?.(); }, /** Force a heartbeat write now — useful right before a long blocking op. */ heartbeat(): void { + if (!started) return; try { writeOwnRow(null); } catch (error) { logger.warn("process_registry.heartbeat_failed", { pid, role, error: error instanceof Error ? error.message : String(error), }); } },Also applies to: 107-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/runtime/processRegistryService.ts` around lines 92 - 104, The heartbeat and other periodic writes call writeOwnRow even after stop() or before start(), causing stale rows; add and check a service-running flag (e.g., this.started or isRunning) inside the process registry instance and only call writeOwnRow from the heartbeatTimer callback and the other periodic/initial write paths if that flag is true, set the flag to true in start() and false in stop(), and ensure heartbeatTimer is cleared/unref'd in stop() so no writes occur after stop; update references to heartbeatTimer, writeOwnRow, start(), and stop() to use this guard.apps/desktop/src/renderer/components/app/CrossRepoPrBanner.tsx-150-169 (1)
150-169:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOffset this banner to avoid collision with the clipboard deeplink banner.
This banner uses the same fixed top-center anchor as
ClipboardDeeplinkBanner, so it can be partially/fully obscured when both are active.💡 Proposed fix
- top: 14, + top: 56,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/app/CrossRepoPrBanner.tsx` around lines 150 - 169, The CrossRepoPrBanner's fixed top-center positioning collides with ClipboardDeeplinkBanner; modify CrossRepoPrBanner (the JSX div with style object) to account for the other banner by applying a dynamic top offset instead of a hardcoded top:14. Implement a small utility/prop or read a shared layout flag (e.g., isClipboardDeeplinkVisible) and set top to a larger value or use calc() (e.g., top: isClipboardDeeplinkVisible ? 56 : 14 or top: "calc(14px + var(--clipboard-banner-height))") so the banners stack without overlap; update the component to accept/derive that flag from parent state or a shared context used by ClipboardDeeplinkBanner.apps/desktop/src/renderer/components/app/CrossRepoPrBanner.tsx-130-144 (1)
130-144:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDo not dismiss on failed project switch.
switchProjectalways callsdismiss(), even whenopenRepofails, which removes the quick retry affordance.💡 Proposed fix
const switchProject = async () => { if (!matchingProject) { dismiss(); return; } const openRepo = window.ade?.project?.openRepo; - if (typeof openRepo === "function") { - try { - await openRepo({ rootPath: matchingProject.rootPath }); - } catch { - // ignore; user can retry from the recent-projects UI - } - } - dismiss(); + if (typeof openRepo !== "function") { + dismiss(); + return; + } + try { + await openRepo({ rootPath: matchingProject.rootPath }); + dismiss(); + } catch { + // keep banner visible so the user can retry + } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/app/CrossRepoPrBanner.tsx` around lines 130 - 144, switchProject currently always calls dismiss(), which removes the retry affordance even when openRepo fails; change the control flow so dismiss() is only invoked on the no-matchingProject path or after a successful openRepo call. Concretely, keep the early return when !matchingProject, then if typeof window.ade?.project?.openRepo === "function" await openRepo(...) inside try and call dismiss() in the try (not after the try/catch); in the catch do not call dismiss so the UI can retry; if openRepo is not a function keep the original dismiss() path. Ensure references to switchProject, matchingProject, openRepo, and dismiss are updated accordingly.apps/desktop/src/renderer/components/app/InboundDeeplinkModal.tsx-73-81 (1)
73-81:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear stale
preflightstate in the branch-only path.At Line 73, the branch-only early return sets
errorbut does not clear a previously loadedpreflight, so reused modal instances can render stale PR metadata from an older target.💡 Proposed fix
if (!target.prNumber) { + setPreflight(null); // Branch-only deeplinks without a PR number aren't supported by the // existing preflight (which is PR-centric). Surface a clear message // so the receiver knows what to do. setError( "This deeplink references a branch without an associated PR. Open the PR in GitHub first, or share the PR deeplink instead.", ); setLoading(false); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/app/InboundDeeplinkModal.tsx` around lines 73 - 81, The branch-only early-return in InboundDeeplinkModal sets error and loading but does not clear any previously loaded preflight state, causing stale PR metadata to persist; update that branch-only path to also clear the preflight state (e.g. call the preflight setter such as setPreflight(null) or setPreflight(undefined)) before setLoading(false) and return so reused modal instances don't render old preflight data.apps/ios/ADE/Services/SyncService.swift-7537-7543 (1)
7537-7543:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
openDeeplinkwhen URL is missing to avoid silent command drops.
sendRemoteCommandstill sendsdeeplinks.openwith empty args ifpayload["url"]is absent/blank, and failures are swallowed later. Early-return here to avoid dispatching an invalid command.💡 Suggested fix
case .openDeeplink: // Desktop `deeplinks.open` expects a `url` arg — the same `ade://...` // string the user tapped on iOS. We pass it through verbatim so the // host can reuse its own router. - if let url = payload["url"] as? String, !url.isEmpty { - args["url"] = url - } + guard let url = (payload["url"] as? String)? + .trimmingCharacters(in: .whitespacesAndNewlines), + !url.isEmpty else { + return + } + args["url"] = url🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ios/ADE/Services/SyncService.swift` around lines 7537 - 7543, In the .openDeeplink branch inside SyncService (where sendRemoteCommand is prepared), guard that payload["url"] is a non-empty String and early-return (or skip dispatch) if it's missing/blank so you do not call sendRemoteCommand with empty args; update the code around the case .openDeeplink handling to validate url up-front (the existing if let url = payload["url"] as? String, !url.isEmpty) and otherwise return/continue before constructing args and calling sendRemoteCommand so invalid commands are not dispatched or silently dropped.apps/ios/ADE/Views/Deeplinks/SendToMacCard.swift-38-43 (1)
38-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve full branch paths in repo deeplink parsing.
Line 43 uses only
parts[3]forbranch, so branch names likefeature/fooare truncated tofeaturein the UI summary.Suggested fix
- case "repo": - if parts.count >= 4, - parts[2].lowercased() == "branch", - !parts[0].isEmpty, - !parts[1].isEmpty, - !parts[3].isEmpty { - self.kind = .repoBranch(owner: parts[0], repo: parts[1], branch: parts[3]) + case "repo": + if parts.count >= 4, + parts[2].lowercased() == "branch", + !parts[0].isEmpty, + !parts[1].isEmpty { + let branch = parts.dropFirst(3).joined(separator: "/") + if !branch.isEmpty { + self.kind = .repoBranch(owner: parts[0], repo: parts[1], branch: branch) + } else { + self.kind = .other + } } else { self.kind = .other }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ios/ADE/Views/Deeplinks/SendToMacCard.swift` around lines 38 - 43, The deeplink parsing currently sets branch to only parts[3], which drops any slashes in branch names (e.g., feature/foo); in SendToMacCard.swift where you assign self.kind = .repoBranch(owner: parts[0], repo: parts[1], branch: parts[3]), change the assignment to join all path segments from index 3 through the end (e.g., join parts[3...] with "/" ) so the full branch path is preserved in the UI summary while keeping the existing parts.count >= 4 validation.
🧹 Nitpick comments (4)
apps/ade-cli/src/tuiClient/__tests__/deeplinkKeybind.test.ts (1)
14-14: ⚡ Quick winUse camelCase for the local UUID constant.
Line 14 uses
LANE_UUID; rename it tolaneUuidto match TS naming conventions in this repo.Suggested rename
-const LANE_UUID = "550e8400-e29b-41d4-a716-446655440000"; +const laneUuid = "550e8400-e29b-41d4-a716-446655440000"; ... - const row: DeeplinkRow = { kind: "lane", lane: { id: LANE_UUID } }; - expect(buildDeeplinkForRow(row)).toBe(`ade://lane/${LANE_UUID}`); + const row: DeeplinkRow = { kind: "lane", lane: { id: laneUuid } }; + expect(buildDeeplinkForRow(row)).toBe(`ade://lane/${laneUuid}`); ... - { kind: "lane", lane: { id: LANE_UUID } }, + { kind: "lane", lane: { id: laneUuid } }, ... - expect(recorded.url).toBe(`ade://lane/${LANE_UUID}`); + expect(recorded.url).toBe(`ade://lane/${laneUuid}`);As per coding guidelines
**/*.{ts,tsx,js,jsx}: Use camelCase for variable names in TypeScript/JavaScript files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/__tests__/deeplinkKeybind.test.ts` at line 14, The constant LANE_UUID in deeplinkKeybind.test.ts should be renamed to camelCase (laneUuid); update the declaration and all usages of LANE_UUID within the file (e.g., any tests referencing LANE_UUID) to laneUuid so it follows the repo's TypeScript naming conventions and avoids lint failures.apps/ade-cli/src/tuiClient/aggregate.ts (1)
338-347: ⚡ Quick winRename runtime suppression constant to camelCase.
Use camelCase for this variable name (e.g.,
suppressedRuntimeActivities) to match project TS naming rules.Suggested rename
-const SUPPRESSED_RUNTIME_ACTIVITIES = new Set([ +const suppressedRuntimeActivities = new Set([ ... - if (SUPPRESSED_RUNTIME_ACTIVITIES.has(activity)) return null; + if (suppressedRuntimeActivities.has(activity)) return null;As per coding guidelines
**/*.{ts,tsx,js,jsx}: Use camelCase for variable names in TypeScript/JavaScript files.Also applies to: 352-352
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/aggregate.ts` around lines 338 - 347, Rename the constant SUPPRESSED_RUNTIME_ACTIVITIES to camelCase suppressedRuntimeActivities and update every usage in this module (and any re-exports/imports) to the new identifier; adjust any references such as where it's read/iterated (e.g., in aggregate.ts functions that check or iterate this set) to use suppressedRuntimeActivities and ensure TypeScript imports/exports remain consistent so the symbol compiles under the new name.apps/desktop/src/main/services/sessions/sessionService.ts (1)
492-543: 💤 Low valueClarify behavior when
liveOwnerPidsis provided as an empty Set vs. undefined.The current logic treats both
undefinedand an emptySetidentically—both result inownerGuardSql = " and owner_pid is null", reconciling only sessions without an owner.This is likely intentional (conservative default), but consider whether an explicitly empty
liveOwnerPidsSet (meaning "no processes are alive") should instead reconcile all sessions regardless ofowner_pid. If current behavior is correct, a brief doc comment clarifying the distinction would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/sessions/sessionService.ts` around lines 492 - 543, The function reconcileStaleRunningSessions conflates undefined liveOwnerPids (meaning "only ownerless sessions") and an explicitly empty Set (which should mean "no live owners, so reconcile all sessions"); change the owner guard logic in reconcileStaleRunningSessions so: if liveOwnerPids === undefined keep ownerGuardSql = " and owner_pid is null"; else build ownerParams = normalized list from liveOwnerPids and if ownerParams.length === 0 set ownerGuardSql = "" (no owner constraint) so all running sessions are reconciled; otherwise keep ownerGuardSql = ` and (owner_pid is null or owner_pid not in (...))`; ensure params = [...normalizedExcludedToolTypes, ...ownerParams] and both the select and update use the same whereSql and params order.apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (1)
157-160: ⚡ Quick winUse camelCase for new toolbar class constants.
Please rename
CHAT_TOOLBAR_ACTION_BASEandCHAT_TOOLBAR_ACTION_IDLEto camelCase to match repo naming rules for TS/JS variables.As per coding guidelines,
**/*.{ts,tsx,js,jsx}: Use camelCase for variable names in TypeScript/JavaScript files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines 157 - 160, Rename the toolbar class constants CHAT_TOOLBAR_ACTION_BASE and CHAT_TOOLBAR_ACTION_IDLE to camelCase (e.g. chatToolbarActionBase and chatToolbarActionIdle) throughout AgentChatPane and any other files that import or reference them; update their declarations in AgentChatPane.tsx and replace all usages (including JSX className concatenations, exports, and any tests) to the new camelCase identifiers to satisfy the TS/JS naming guidelines while preserving the string values unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8bfad10-a340-48cf-be15-c3873e98f2f2
⛔ Files ignored due to path filters (7)
apps/ios/ADE.xcodeproj/project.pbxprojis excluded by!**/*.xcodeproj/project.pbxprojdocs/ARCHITECTURE.mdis excluded by!docs/**docs/features/ade-code/README.mdis excluded by!docs/**docs/features/deeplinks/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**goal.mdis excluded by!*.md
📒 Files selected for processing (142)
apps/ade-cli/README.mdapps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/bootstrap.test.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/commands/deeplinks.test.tsapps/ade-cli/src/commands/deeplinks.tsapps/ade-cli/src/eventBuffer.tsapps/ade-cli/src/lib/clipboard.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/services/sync/syncService.tsapps/ade-cli/src/tuiClient/__tests__/ChatView.test.tsxapps/ade-cli/src/tuiClient/__tests__/Drawer.test.tsxapps/ade-cli/src/tuiClient/__tests__/GridMiniMap.test.tsapps/ade-cli/src/tuiClient/__tests__/adeApi.test.tsapps/ade-cli/src/tuiClient/__tests__/aggregate.test.tsapps/ade-cli/src/tuiClient/__tests__/appInput.test.tsapps/ade-cli/src/tuiClient/__tests__/deeplinkKeybind.test.tsapps/ade-cli/src/tuiClient/__tests__/format.test.tsapps/ade-cli/src/tuiClient/__tests__/hitTestRegistry.test.tsapps/ade-cli/src/tuiClient/__tests__/multiChatLayout.test.tsapps/ade-cli/src/tuiClient/adeApi.tsapps/ade-cli/src/tuiClient/aggregate.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/cli.tsxapps/ade-cli/src/tuiClient/components/AddChatMode.tsxapps/ade-cli/src/tuiClient/components/ChatView.tsxapps/ade-cli/src/tuiClient/components/Drawer.tsxapps/ade-cli/src/tuiClient/components/FooterControls.tsxapps/ade-cli/src/tuiClient/components/GridMiniMap.tsxapps/ade-cli/src/tuiClient/components/MultiChatGrid.tsxapps/ade-cli/src/tuiClient/components/RightPane.tsxapps/ade-cli/src/tuiClient/deeplinkRow.tsapps/ade-cli/src/tuiClient/format.tsapps/ade-cli/src/tuiClient/hitTestRegistry.tsapps/ade-cli/src/tuiClient/keybindings/index.tsapps/ade-cli/src/tuiClient/multiChatLayout.tsapps/desktop/resources/agent-skills/ade-deeplinks/SKILL.mdapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/cto/linearClient.tsapps/desktop/src/main/services/cto/linearLaneCardService.test.tsapps/desktop/src/main/services/cto/linearLaneCardService.tsapps/desktop/src/main/services/deeplinks/protocolHandler.test.tsapps/desktop/src/main/services/deeplinks/protocolHandler.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/ipc/runtimeBridge.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.test.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.tsapps/desktop/src/main/services/review/reviewContextBuilder.test.tsapps/desktop/src/main/services/review/reviewService.test.tsapps/desktop/src/main/services/review/reviewService.tsapps/desktop/src/main/services/review/reviewTargetMaterializer.test.tsapps/desktop/src/main/services/runtime/processRegistryService.test.tsapps/desktop/src/main/services/runtime/processRegistryService.tsapps/desktop/src/main/services/sessions/sessionService.test.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/App.tsxapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/app/ClipboardDeeplinkBanner.tsxapps/desktop/src/renderer/components/app/CrossRepoPrBanner.tsxapps/desktop/src/renderer/components/app/InboundDeeplinkModal.tsxapps/desktop/src/renderer/components/app/LinearIssueBrowser.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatComposerShell.tsxapps/desktop/src/renderer/components/chat/ChatGitToolbar.test.tsxapps/desktop/src/renderer/components/chat/ChatGitToolbar.tsxapps/desktop/src/renderer/components/chat/ChatSubagentsPanel.tsxapps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsxapps/desktop/src/renderer/components/chat/ChatWorkLogBlock.tsxapps/desktop/src/renderer/components/chat/chatSurfaceTheme.tsapps/desktop/src/renderer/components/chat/chatTranscriptRows.test.tsapps/desktop/src/renderer/components/chat/chatTranscriptRows.tsapps/desktop/src/renderer/components/chat/codex/CodexOpenInCliButton.tsxapps/desktop/src/renderer/components/files/FilesPage.tsxapps/desktop/src/renderer/components/lanes/LaneContextMenu.tsxapps/desktop/src/renderer/components/lanes/LaneWorkPane.tsxapps/desktop/src/renderer/components/lanes/LanesPage.test.tsapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/laneDesignTokens.tsapps/desktop/src/renderer/components/lanes/laneUtils.tsapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.test.tsapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/onboarding/HelpMenu.tsxapps/desktop/src/renderer/components/review/ReviewPage.test.tsxapps/desktop/src/renderer/components/review/ReviewPage.tsxapps/desktop/src/renderer/components/review/reviewTypes.tsapps/desktop/src/renderer/components/settings/ChatAppearancePreview.test.tsxapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.test.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/components/ui/TabBackground.tsxapps/desktop/src/renderer/components/usage/HeaderUsageControl.tsxapps/desktop/src/renderer/index.cssapps/desktop/src/renderer/onboarding/tours/laneWorkPaneHighlightsTour.tsapps/desktop/src/renderer/onboarding/tours/laneWorkPaneTour.tsapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/adeDeeplinkFooter.test.tsapps/desktop/src/shared/adeDeeplinkFooter.tsapps/desktop/src/shared/deeplinks.test.tsapps/desktop/src/shared/deeplinks.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/core.tsapps/desktop/src/shared/types/remoteRuntime.tsapps/desktop/src/shared/types/review.tsapps/desktop/src/shared/types/sessions.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/App/ADEApp.swiftapps/ios/ADE/App/DeepLinkRouter.swiftapps/ios/ADE/Resources/DatabaseBootstrap.sqlapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Deeplinks/SendToMacCard.swiftapps/web/api/open.tsapps/web/api/tsconfig.jsonapps/web/src/app/SiteRoutes.tsxapps/web/src/app/pages/OpenPage.tsxapps/web/vercel.json
💤 Files with no reviewable changes (7)
- apps/desktop/src/shared/types/review.ts
- apps/desktop/src/renderer/components/lanes/laneUtils.ts
- apps/desktop/src/renderer/components/review/reviewTypes.ts
- apps/desktop/src/renderer/components/files/FilesPage.tsx
- apps/desktop/src/main/services/review/reviewTargetMaterializer.test.ts
- apps/desktop/src/main/services/review/reviewContextBuilder.test.ts
- apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
May 21, 2026 08:31
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
May 21, 2026 08:57
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 (2)
apps/desktop/src/main/services/ipc/runtimeBridge.ts (1)
264-283:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate forwarded runtime events by
requestKey, not onlybindingKey.After this change, subscriptions are keyed by
requestKey, but forwarded events are still validated only againstbindingKey. A late callback from a prior category subscription can still pass the guard and emit stale events.Suggested fix
-function sendRuntimeEvent( - sender: WebContents, - bindingKey: string, - event: RemoteRuntimeBufferedEvent, -): void => { +function sendRuntimeEvent( + sender: WebContents, + bindingKey: string, + requestKey: string, + event: RemoteRuntimeBufferedEvent, +): void => { const existing = runtimeEventSubscriptions.get(sender.id); - if (!existing || existing.bindingKey !== bindingKey || sender.isDestroyed()) + if ( + !existing || + existing.bindingKey !== bindingKey || + existing.requestKey !== requestKey || + sender.isDestroyed() + ) return; const payload: RemoteRuntimeEventNotificationPayload = { bindingKey, event, }; @@ - void subscribe( - (event) => sendRuntimeEvent(sender, bindingKey, event), + void subscribe( + (event) => sendRuntimeEvent(sender, bindingKey, requestKey, event), onEnded, )Also applies to: 286-300
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/ipc/runtimeBridge.ts` around lines 264 - 283, ensureRuntimeEventSubscription registers subscriptions keyed by requestKey but the forwarded events are only validated against bindingKey, allowing stale callbacks to emit; update the subscribe callback and the onEnded/cleanup logic to verify both requestKey and bindingKey before calling sendRuntimeEvent and before deleting entries. Specifically, in ensureRuntimeEventSubscription change the subscribe success callback (the first arg to RuntimeEventSubscribe) to capture the local requestKey and bindingKey and, on each event, fetch runtimeEventSubscriptions.get(sender.id) and only call sendRuntimeEvent(sender, bindingKey, event) if current?.requestKey === requestKey && current?.bindingKey === bindingKey; apply the same dual-key guard in the corresponding subscription code path around lines 286-300 and ensure cleanupRuntimeEventSubscription/onEnded use the requestKey match as well.apps/ade-cli/src/tuiClient/app.tsx (1)
2539-2545:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the transcript mirror until the new session is actually loaded.
When
activeSessionIdflips,eventsstill holds the previous session untilrefreshState()hydrates the new one. This effect immediately copies that stale array intoeventsBySessionId[activeSessionId], so a newly focused tile can render the wrong transcript and keep it if the reload fails.Suggested fix
useEffect(() => { - if (!activeSessionId) return; + if (!activeSessionId || loadedSessionIdRef.current !== activeSessionId) return; setEventsBySessionId((prev) => { if (prev[activeSessionId] === events) return prev; return { ...prev, [activeSessionId]: events }; }); }, [activeSessionId, events]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/tuiClient/app.tsx` around lines 2539 - 2545, The effect is copying stale `events` into `eventsBySessionId` when `activeSessionId` flips; add a guard so we only mirror `events` into `eventsBySessionId` if those `events` were actually hydrated for the current session. Implement this by introducing a ref (e.g., lastHydratedSessionRef) that you set when the refresh/hydration finishes (or when the source that provides `events` confirms it's for a given session), then in the existing useEffect (the one referencing activeSessionId, events, setEventsBySessionId) return early unless lastHydratedSessionRef.current === activeSessionId; keep the existing identity check (prev[activeSessionId] === events) otherwise so you only update when the events belong to the currently loaded session.
🧹 Nitpick comments (2)
apps/ios/ADE/Services/SyncService.swift (1)
7541-7546: ⚡ Quick winAllowlist deeplink formats before forwarding to desktop.
This currently forwards any non-empty string. Restrict to supported deeplink forms (
ade://...andhttps://ade.app/open...) to harden the remote-command path.♻️ Suggested patch
guard let url = (payload["url"] as? String)? .trimmingCharacters(in: .whitespacesAndNewlines), - !url.isEmpty else { + !url.isEmpty, + let components = URLComponents(string: url), + ( + components.scheme?.lowercased() == "ade" + || ( + components.scheme?.lowercased() == "https" + && components.host?.lowercased() == "ade.app" + && components.path == "/open" + ) + ) + else { return } args["url"] = url🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ios/ADE/Services/SyncService.swift` around lines 7541 - 7546, Currently any non-empty payload["url"] is forwarded; restrict to approved deeplink formats by parsing the trimmed url (the local variable url derived from payload["url"]) using URLComponents and validating either scheme == "ade" OR (scheme == "https" && host == "ade.app" && path.starts(with: "/open")). If validation fails, return early instead of setting args["url"]. Update the guard/validation around the url extraction (the block that assigns args["url"] = url) to perform this allowlist check before forwarding.apps/ade-cli/src/adeRpcServer.test.ts (1)
1288-1300: ⚡ Quick winExpand external PTY gating coverage to match the test intent.
On Line 1288, the test says “methods” but only asserts
pty.createis blocked. Add assertions forpty.sendToSession,pty.write,pty.resize,pty.dispose, andpty.listso permission regressions on any direct PTY RPC are caught.Suggested test shape
it("hides direct PTY RPC methods from external sessions", async () => { const { runtime } = createRuntime(); const handler = createAdeRpcRequestHandler({ runtime, serverVersion: "test" }); await initialize(handler, { role: "external" }); - await expect(handler({ - jsonrpc: "2.0", - id: 2, - method: "pty.create", - params: { args: { laneId: "lane-1", title: "Claude", cols: 120, rows: 40 } }, - })).rejects.toMatchObject({ code: JsonRpcErrorCode.methodNotFound }); - expect(runtime.ptyService.create).not.toHaveBeenCalled(); + const blocked = [ + { + method: "pty.create", + params: { args: { laneId: "lane-1", title: "Claude", cols: 120, rows: 40 } }, + spy: runtime.ptyService.create, + }, + { + method: "pty.sendToSession", + params: { args: { sessionId: "session-1", text: "continue" } }, + spy: runtime.ptyService.sendToSession, + }, + { method: "pty.write", params: { args: { ptyId: "pty-1", data: "x" } }, spy: runtime.ptyService.write }, + { method: "pty.resize", params: { args: { ptyId: "pty-1", cols: 100, rows: 30 } }, spy: runtime.ptyService.resize }, + { method: "pty.dispose", params: { args: { ptyId: "pty-1", sessionId: "session-1" } }, spy: runtime.ptyService.dispose }, + { method: "pty.list", params: { args: { laneId: "lane-1", limit: 20 } }, spy: runtime.ptyService.list }, + ] as const; + + for (const [index, rpc] of blocked.entries()) { + await expect(handler({ + jsonrpc: "2.0", + id: 2 + index, + method: rpc.method, + params: rpc.params, + })).rejects.toMatchObject({ code: JsonRpcErrorCode.methodNotFound }); + expect(rpc.spy).not.toHaveBeenCalled(); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/adeRpcServer.test.ts` around lines 1288 - 1300, The test currently only checks that "pty.create" is blocked for external sessions; extend it to assert that "pty.sendToSession", "pty.write", "pty.resize", "pty.dispose", and "pty.list" are also rejected with JsonRpcErrorCode.methodNotFound by invoking handler with each method name (using the same jsonrpc/id/params shape) and expecting rejects.toMatchObject({ code: JsonRpcErrorCode.methodNotFound }); also add ensures that runtime.ptyService.sendToSession, .write, .resize, .dispose, and .list have not been called (similar to the existing expect for runtime.ptyService.create) to fully cover direct PTY gating in createAdeRpcRequestHandler/handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/tuiClient/app.tsx`:
- Around line 7976-7979: The Ctrl+G handler for starting add-mode is being
shadowed by an earlier Ctrl+G branch that calls editPromptInExternalEditor when
chat input is focused, so startAddMode() never runs; to fix, either choose a
non-colliding chord for the add-mode handler (replace isCtrlInput(input, key,
"g") with another key/chord) or make precedence explicit by moving the pane ===
"chat" && isCtrlInput(input, key, "g") check above the
editPromptInExternalEditor branch or add a disambiguating condition (e.g.,
ensure the editPromptInExternalEditor path checks the specific focus state
before handling Ctrl+G); update references to startAddMode, isCtrlInput,
editPromptInExternalEditor, pane, input, and key accordingly.
In `@apps/desktop/src/main/main.ts`:
- Around line 6005-6028: The current draining of pendingAppNavigationRequests
calls openAdeWindow() during bootstrap and can race with later creation of
initialWindow, causing two windows; update the logic so
dispatchAppNavigationRequest no longer directly calls openAdeWindow during early
bootstrap but instead awaits or reuses the shared initial-window promise used by
the bootstrap (or delay draining until after initialWindow is created).
Concretely, modify dispatchAppNavigationRequest to obtain the app's
single-initial-window via the existing shared promise or getter (referencing
dispatchAppNavigationRequest, openAdeWindow, pendingAppNavigationRequests, and
initialWindow) and use that window result instead of invoking openAdeWindow
directly, or move the loop that splices pendingAppNavigationRequests to run
after initialWindow has been established during bootstrap.
---
Outside diff comments:
In `@apps/ade-cli/src/tuiClient/app.tsx`:
- Around line 2539-2545: The effect is copying stale `events` into
`eventsBySessionId` when `activeSessionId` flips; add a guard so we only mirror
`events` into `eventsBySessionId` if those `events` were actually hydrated for
the current session. Implement this by introducing a ref (e.g.,
lastHydratedSessionRef) that you set when the refresh/hydration finishes (or
when the source that provides `events` confirms it's for a given session), then
in the existing useEffect (the one referencing activeSessionId, events,
setEventsBySessionId) return early unless lastHydratedSessionRef.current ===
activeSessionId; keep the existing identity check (prev[activeSessionId] ===
events) otherwise so you only update when the events belong to the currently
loaded session.
In `@apps/desktop/src/main/services/ipc/runtimeBridge.ts`:
- Around line 264-283: ensureRuntimeEventSubscription registers subscriptions
keyed by requestKey but the forwarded events are only validated against
bindingKey, allowing stale callbacks to emit; update the subscribe callback and
the onEnded/cleanup logic to verify both requestKey and bindingKey before
calling sendRuntimeEvent and before deleting entries. Specifically, in
ensureRuntimeEventSubscription change the subscribe success callback (the first
arg to RuntimeEventSubscribe) to capture the local requestKey and bindingKey
and, on each event, fetch runtimeEventSubscriptions.get(sender.id) and only call
sendRuntimeEvent(sender, bindingKey, event) if current?.requestKey ===
requestKey && current?.bindingKey === bindingKey; apply the same dual-key guard
in the corresponding subscription code path around lines 286-300 and ensure
cleanupRuntimeEventSubscription/onEnded use the requestKey match as well.
---
Nitpick comments:
In `@apps/ade-cli/src/adeRpcServer.test.ts`:
- Around line 1288-1300: The test currently only checks that "pty.create" is
blocked for external sessions; extend it to assert that "pty.sendToSession",
"pty.write", "pty.resize", "pty.dispose", and "pty.list" are also rejected with
JsonRpcErrorCode.methodNotFound by invoking handler with each method name (using
the same jsonrpc/id/params shape) and expecting rejects.toMatchObject({ code:
JsonRpcErrorCode.methodNotFound }); also add ensures that
runtime.ptyService.sendToSession, .write, .resize, .dispose, and .list have not
been called (similar to the existing expect for runtime.ptyService.create) to
fully cover direct PTY gating in createAdeRpcRequestHandler/handler.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 7541-7546: Currently any non-empty payload["url"] is forwarded;
restrict to approved deeplink formats by parsing the trimmed url (the local
variable url derived from payload["url"]) using URLComponents and validating
either scheme == "ade" OR (scheme == "https" && host == "ade.app" &&
path.starts(with: "/open")). If validation fails, return early instead of
setting args["url"]. Update the guard/validation around the url extraction (the
block that assigns args["url"] = url) to perform this allowlist check before
forwarding.
🪄 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: 563acdab-8e57-4d3b-9ee3-da40b21048be
📒 Files selected for processing (33)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/commands/deeplinks.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/tuiClient/__tests__/deeplinkKeybind.test.tsapps/ade-cli/src/tuiClient/aggregate.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/components/Drawer.tsxapps/ade-cli/src/tuiClient/components/GridMiniMap.tsxapps/ade-cli/src/tuiClient/deeplinkRow.tsapps/ade-cli/src/tuiClient/multiChatLayout.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/deeplinks/protocolHandler.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/ipc/runtimeBridge.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/runtime/processRegistryService.test.tsapps/desktop/src/main/services/runtime/processRegistryService.tsapps/desktop/src/main/services/sessions/sessionService.test.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/renderer/components/app/ClipboardDeeplinkBanner.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/shared/adeDeeplinkFooter.tsapps/desktop/src/shared/types/sessions.tsapps/ios/ADE/App/DeepLinkRouter.swiftapps/ios/ADE/Resources/DatabaseBootstrap.sqlapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Deeplinks/SendToMacCard.swiftapps/web/src/app/pages/OpenPage.tsx
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
May 21, 2026 09:23
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Documentation
Greptile Summary
This PR ships the complete ADE deeplinks system: an
ade://protocol handler in the Electron main process, CLI subcommands (ade open,ade link,ade linear install), a clipboard-sniffing banner, an inbound branch-deeplink modal, a multi-chat tiled TUI view, and an iOS "Send to Mac" confirmation card backed by a Vercel/openserverless function that rewrites OpenGraph tags for rich unfurls.protocolHandler.tsregisters theade://scheme;adeDeeplinkFooter.tsidempotently appends a branded footer to all newly created PR bodies;laneService.tswraps allgit worktreemutations in anAsyncLocalStorage-backed serial queue to prevent concurrent worktree races.deeplinks.tsaddsade open/ade link/ade linear install;runLinearInstallmerges with existing~/.linear/coding-tools.jsonbut skips the promised backup when the file exists with malformed JSON.DeepLinkRouter.swiftroutes new cross-machine deeplink shapes to aSendToMacCardsheet;apps/web/api/open.tsrewrites OG meta tags from validated, HTML-escaped query parameters.Confidence Score: 3/5
Two defects need fixing before merge: the BRANCH_BAD_RE regex breaks every branch-based deeplink, and runLinearInstall silently destroys a user config file when JSON parse fails.
The BRANCH_BAD_RE bug means isValidBranch rejects any branch containing a hyphen, breaking all branch and repo deeplinks end-to-end and causing the new tests to fail. Separately, runLinearInstall backs up coding-tools.json only when JSON.parse succeeds; if the file is malformed the backup is skipped and the file is silently overwritten. Both are regressions on new code introduced in this PR.
apps/desktop/src/shared/deeplinks.ts (BRANCH_BAD_RE regex) and apps/ade-cli/src/commands/deeplinks.ts (runLinearInstall backup logic)
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant OS participant Desktop as Electron Main participant Renderer participant CLI as ADE CLI participant iOS Note over User,iOS: Outbound sharing User->>CLI: ade link branch owner/repo feat-x CLI->>CLI: buildDeeplink to https URL CLI-->>User: URL copied to clipboard Note over User,iOS: Inbound desktop cold-start User->>OS: open ade://repo/owner/repo/branch/feat-x OS->>Desktop: open-url event Desktop->>Desktop: parseDeeplink to AppNavigationTarget Desktop->>Renderer: IPC appNavigate branch target Renderer->>Renderer: InboundDeeplinkModal shown User->>Renderer: Confirm Renderer->>Desktop: IPC createLaneFromPrBranch Desktop-->>Renderer: lane created navigate /lanes Note over User,iOS: iOS Send to Mac User->>iOS: tap ade://lane/uuid link iOS->>iOS: DeepLinkRouter posts adeSendToMacRequested iOS->>User: SendToMacCard sheet User->>iOS: Confirm send iOS->>Desktop: SyncService relay Note over User,iOS: Clipboard detection User->>Renderer: paste ade:// URL then focus window Renderer->>Renderer: ClipboardDeeplinkBanner appears User->>Renderer: click Open Renderer->>OS: openExternal ade:// URL OS->>Desktop: open-url event navigateComments Outside Diff (2)
apps/desktop/src/shared/deeplinks.ts, line 63 (link)BRANCH_BAD_RErejects hyphenated branch namesThe character class
[ -]contains a space and a literal hyphen (ECMAScript non-unicode mode treats-at the end of a character class as a literal). This meansisValidBranch("feat-deeplinks")returnsfalse, causingparseDeeplinkto reject the overwhelming majority of real-world branch names (e.g.feat-deeplinks,fix-bug-123). The new tests asserting thatfeat-deeplinksparses successfully will also fail. Based on the comment ("traversal + control chars"), the intended fix is to remove the hyphen: change[ -]to[ ](space only).Prompt To Fix With AI
apps/desktop/src/shared/deeplinks.ts, line 63 (link)BRANCH_BAD_RErejects all hyphenated branch namesThe character class
[ -]contains a space and a literal hyphen (in ECMAScript non-unicode mode,-at the end of a class is treated as a literal, not a range). This meansisValidBranchrejects any branch containing a-, so names likefeat-deeplinks,fix-bug-123, andusers/arul/feat-xall returnfalse. The existing test asserting thatfeat-deeplinksparses successfully will fail. The fix is to remove the hyphen from the character class:const BRANCH_BAD_RE = /(^|\/)\.\.($|\/)| /;Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "ship: address follow-up review feedback" | Re-trigger Greptile