Deeplinks Overhaul#713
Merged
Merged
Conversation
…ening A query whose kinds array contains only invalid entries (e.g. ["termnal"]) previously fell back to the default all-kind set — silently broadening the search. Mirror the inline kind:bogus reject: supplied + nothing valid left means empty results. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ion ladder (desktop) - Shared contract: session links carry event=<seq> (chat) / offset=<bytes> (terminal) anchors; new file target (repo-relative path + line + lane); new commit/artifact targets; portable envelope (repo/branch/pr/linear query params) on machine-local targets, parsed leniently. - Anchors consumed end-to-end: chat list scrolls to the anchored event (envelope.sequence match, ordinal fallback on full history, bounded older-history paging, highlight); terminal replay scrolls by byte fraction when the whole transcript is loaded; Files editor reveals the anchored line. Search emits envelope.sequence in chat links and canonical ade://file links (index schema 2->3, disposable rebuild). - Resolution ladder for session/lane links: local -> open exactly; repo matches another known project (new project.findForRepo IPC + repoProjectResolver over recents' git configs) -> switch-project card; foreign machine -> fallback-only card (create lane from branch, open PR on GitHub, open Linear issue). InboundDeeplinkModal generalized; no request-access path by design. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nd-to-Mac, link minting - https /open session links navigate locally on the phone; file/commit/ artifact shapes validate then route to Send-to-Mac; session anchors carried on WorkSessionNavigationRequest (no scroll hook yet on iOS). - Universal Links: applinks:ade-app.dev entitlement, NSUserActivity routing, AASA served from apps/web/.well-known (appID VQ372F39G6.com.ade.ios, /open* and /pair* only) with JSON content-type. - Envelope-aware Swift link builder; lane/session/PR copy-link actions mint https links with envelopes; Send-to-Mac card offers GitHub PR / Linear fallbacks from the envelope. Web /open page describes the new target types. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…embed sweep - commit target dispatches to the owning lane (commitSha param) with the same resolution ladder as sessions/lanes; foreign fallback offers the GitHub commit URL. artifact links open the history surface (no focused- artifact mechanism exists yet). Palette commit results deep-route too. - ade link gains file/commit/artifact targets plus mint-time envelope enrichment (lane branch, PR number, Linear issue) with --no-envelope; round-trip re-emit handles all new forms. ade-deeplinks skill doc updated. - Auto-embed: Linear lane cards and desktop copy-link affordances (lane context menu, session copy, TUI Ctrl+Y) now mint envelope-carrying links. Search emits canonical commit/artifact links and repo/branch envelopes on session links via an optional repoSlug dep (index schema 3->4). - Recovered from an out-of-process lane-branch reset that wiped the working tree mid-task: restored A/B content (kinds guard, sequence anchors, canonical file links, palette anchor pass, regression tests) and re-added the named findForRepo types. Backup refs: backup/pre-incident, backup/c-snapshot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… ladder, iOS universal links Also restores the contract-file doc comments and the parser edge-case tests (traversal normalization, malformed anchors, envelope leniency). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-sha retry, envelope/link hygiene Dual-review synthesis fixes: - app/navigate RPC file targets now validated with the shared repo-relative rules (exported isValidRepoRelativePath/isValidCommitSha) + defense-in-depth check in renderer dispatch — an RPC caller could previously smuggle ../../ paths past the URL parser into openExternalPath. - Work URL anchor apply-once guard keyed by session AND event/offset so re-opening the same session at a different anchor re-positions. - Commit deeplink keeps laneId+commitSha through switch-project retries instead of dropping to a bare /lanes. - Lane search results now carry the portable envelope (envelopeForLane). - ade link refuses to mint links the shared parser would reject (round-trip gate). - project.findForRepo IPC rewired through the tested mtime-cached resolver (was an inline reimplementation spawning git per recent project). - Parser branch/pr assembly unified into shared helpers across both URL forms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Pin the quality gate: app/navigate file-traversal + malformed-sha rejection (adeRpcServer), ade link round-trip refusal (CLI), lane search envelope (searchService). - Consolidate services/deeplinks to one test file (dispatch contract merged into protocolHandler.test.ts). - Parity residuals: search README deepLink prose, ios-companion universal-link notes, ade-cli README CLI-surface inventory. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR adds file, commit, and artifact deeplink target kinds plus an optional portable "envelope" (repo/branch/PR/Linear metadata) across the shared deeplink schema, desktop main/renderer, ade-cli RPC/CLI, iOS parsing/UI, and web open-page handling. It also adds chat/terminal session-anchor scrolling and a project-by-repo lookup service. ChangesADE deeplink envelope and target expansion
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
July 7, 2026 01:26
…l-53b28aac # Conflicts: # apps/web/public/.well-known/apple-app-site-association
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…main The lane-branch-reset incident left three files rebuilt from a pre-#710 base, which read to git as intentional reverts of relay-everywhere code (Greptile P1s: relay URL missing from handshake, non-host runtime owning the relay). 3-way remerge (base=#709, ours=main, theirs=branch) so main's relay behavior returns while the deeplink additions stay. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 845e736425
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+164
to
+165
| if resolvePrId(from: "\(number)") != nil || (query["repo"]?.isEmpty ?? true) { | ||
| post(kind: "pr", identifier: "\(number)", prNumber: number) |
There was a problem hiding this comment.
For https://ade-app.dev/open?type=pr&repo=other/repo&number=N, this branch opens a local PR whenever any synced PR has number N, because resolvePrId(from:) only looks up snapshot.prs.first(where: { $0.number == number }) and ignores the repo query. PR numbers are only unique within a repository, so a universal link for another repo can navigate iOS to the wrong local PR instead of showing the Send to Mac handoff; require the resolved PR's owner/name to match the query before posting local navigation.
Useful? React with 👍 / 👎.
| const laneId = flags.valued.get("lane"); | ||
| return plan({ | ||
| kind: "file", | ||
| path: filePath, |
There was a problem hiding this comment.
When ade link file ... --ade is used, the raw filePath is passed into the path-form URL before validation. WHATWG URL parsing normalizes dot segments and leading slashes before finishLink round-trips it, so inputs like ../secret or /etc/passwd are accepted and copied as links that resolve to secret or etc/passwd inside the repo instead of being rejected. Validate the original filePath with the shared repo-relative path rule before building the target.
Useful? React with 👍 / 👎.
…iOS; validate ade link file paths before building - iOS: https pr links carrying a repo param no longer resolve a local PR by bare number (numbers are only unique per repo and the snapshot has no repo identity) — they route to Send-to-Mac where the desktop ladder verifies. - ade link file validates the input path with the shared repo-relative rule BEFORE building: the ade:// path form URL-normalizes dot segments, so the post-build round-trip gate alone would accept ../secret as a link to a different in-repo path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d7369ac92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+253
to
+256
| const handled = await onDispatchTarget(currentTarget.original, { | ||
| suppressUnresolved: attempt < 4, | ||
| forceLocal: attempt === 4, | ||
| }); |
There was a problem hiding this comment.
In the "Switch project and open" path, onDispatchTarget is the callback captured by this modal before switchToPath changes the active project, so it still closes over the old project's lanes and project values when the retry loop runs. For a lane/chat/commit deeplink whose envelope matches another local project, the redispatch is evaluated against the previous project and can navigate to foreign ids or re-open the fallback instead of resolving in the project that was just opened; defer the redispatch to the parent after project state updates, or call through a ref that always points at the latest dispatcher/project state.
Useful? React with 👍 / 👎.
Comment on lines
+1009
to
+1013
| const lane = laneById(target.laneId); | ||
| const root = lane?.worktreePath || project?.rootPath || ""; | ||
| const localPath = root | ||
| ? `${root.replace(/\/+$/, "")}/${target.path.replace(/^\/+/, "")}` | ||
| : target.path; |
There was a problem hiding this comment.
When a file deeplink includes a laneId but the lane list has not loaded yet (for example during cold start or immediately after switching projects), laneById returns null and this falls back to project.rootPath, so ade://file/...?...lane=<uuid> can open the same relative path in the primary project instead of the requested worktree. If an explicit lane is unresolved, wait/retry or surface an unresolved state rather than composing an absolute path from the project root.
Useful? React with 👍 / 👎.
…ot fallback for lane file links - The switch-project card now re-dispatches through a ref that always points at the latest dispatcher, so post-switch retries see the new project's lanes/root instead of the closure captured before switchToPath. - laneById / active-repo / findForRepo comparisons read via refs (same class). - File deeplinks with an explicit lane never degrade to the project root: refresh + bounded wait for the lane list, then open the Files tab bare (or report unhandled during suppressed retries) instead of composing the same relative path in the wrong worktree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
Contributor
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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/renderer/components/app/InboundDeeplinkModal.tsx (1)
390-412: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEarly return leaves
busystuck astrue.
setBusy(true)runs unconditionally at the top ofonConfirm. The newly-addedif (!branchTarget.prNumber) return;(Line 412) doesn't reset it, unlike the other early-return branch just above (which callssetBusy(false)). Currently unreachable in practice sinceisBranchOnlyalready gates this, but if the invariant ever breaks, the modal becomes stuck with both buttons disabled and no error shown.🐛 Proposed fix
- if (!branchTarget.prNumber) return; + if (!branchTarget.prNumber) { + setBusy(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 390 - 412, The early return in onConfirm leaves busy set to true if branchTarget.prNumber is missing, which can trap the modal in a disabled state. Update the onConfirm flow in InboundDeeplinkModal so this guard mirrors the existing branch-only failure path by resetting busy before returning, or move the validation ahead of setBusy(true) if that better fits the control flow. Keep the fix local to onConfirm and preserve the current error handling pattern used with setError and setBusy.apps/desktop/src/renderer/components/files/v2/FilesWorkbench.tsx (1)
170-176: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLine can be misapplied to the wrong file under overlapping external-open requests.
pendingWorkspaceOpendoesn't carry the requested line; the second effect re-reads the liveexternalOpenLineprop when the async open resolves. If a second deeplink (different path/line) arrives before the first'sopenExternalPathRequestround-trip completes, the first request's resolution can pick up the second request'sexternalOpenLine, revealing the wrong line in the wrong file.🐛 Proposed fix: capture line at request time
const [pendingWorkspaceOpen, setPendingWorkspaceOpen] = useState<{ workspaceId: string; path: string | null; pathType: "file" | "directory"; nonce: string; + line?: number; } | null>(null);const openExternalPathRequest = useCallback( - async (absolutePath: string, nonce: string) => { + async (absolutePath: string, nonce: string, line?: number) => { try { const result = await window.ade.files.openExternalPath({ path: absolutePath }); if (result.workspace.kind === "external") { upsertWorkspace(result.workspace); } setWorkspaceId(result.workspace.id); setPendingWorkspaceOpen({ workspaceId: result.workspace.id, path: result.openPath, pathType: result.pathType, nonce, + line, }); } catch (err) { setError(err instanceof Error ? err.message : String(err)); } }, [upsertWorkspace], );useEffect(() => { if (!active || !externalOpenPath) return; const key = `${externalOpenNonce ?? ""}:${externalOpenPath}:${externalOpenLine ?? ""}`; if (handledExternalOpenRef.current === key) return; handledExternalOpenRef.current = key; - void openExternalPathRequest(externalOpenPath, key); - }, [active, externalOpenPath, externalOpenLine, externalOpenNonce, openExternalPathRequest]); + const parsedLine = externalOpenLine && /^\d+$/.test(externalOpenLine) ? Number(externalOpenLine) : undefined; + void openExternalPathRequest(externalOpenPath, key, parsedLine); + }, [active, externalOpenPath, externalOpenLine, externalOpenNonce, openExternalPathRequest]);if (pending.pathType === "file" && pending.path) { - const line = externalOpenLine && /^\d+$/.test(externalOpenLine) - ? Number(externalOpenLine) - : undefined; - void openFile(pending.path, { preview: false, line }); + void openFile(pending.path, { preview: false, line: pending.line }); return; } ... - }, [active, externalOpenLine, loadDirectoryPath, openFile, pendingWorkspaceOpen, refreshRoot, workspaceId]); + }, [active, loadDirectoryPath, openFile, pendingWorkspaceOpen, refreshRoot, workspaceId]);Also applies to: 769-796, 798-823
🤖 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/files/v2/FilesWorkbench.tsx` around lines 170 - 176, Capture the requested line at the time `pendingWorkspaceOpen` is created and carry it through the async external-open flow so `FilesWorkbench` does not re-read the live `externalOpenLine` prop after `openExternalPathRequest` resolves. Update the state shape used by `pendingWorkspaceOpen` and the follow-up effect(s) around the external-open handling to use that stored line when opening/revealing the file, preventing overlapping deeplinks from applying the wrong line to the wrong file.
🧹 Nitpick comments (6)
apps/web/api/open.ts (1)
79-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
positiveIntegerhelper vs.OpenPage.tsx.Nearly identical logic is reimplemented in
apps/web/src/app/pages/OpenPage.tsx(positiveInteger/nonNegativeInteger). Consider extracting to a small shared module underapps/web/srcto avoid drift between the API route and the SPA parser.🤖 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/web/api/open.ts` around lines 79 - 82, The positiveInteger helper logic is duplicated between the API route and OpenPage.tsx, so extract the shared parsing behavior into a small reusable module under apps/web/src and have both the open.ts handler and the OpenPage positiveInteger/nonNegativeInteger helpers import it. Keep the existing function semantics the same, but centralize the implementation so future changes only happen in one place and the API and SPA stay aligned.apps/ade-cli/src/adeRpcServer.test.ts (1)
1040-1103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for envelope normalization and the new
artifactkind.These new tests only cover
filepath rejection/acceptance andcommitsha rejection/normalization. There's no test verifying: (1) that acommit/artifacttarget'senvelope(repoOwner/repoName/branch/prNumber/linearIssue) round-trips through normalization correctly, and (2) basic acceptance/rejection forkind: "artifact"(missingartifactId→ invalidParams; validartifactId→ forwarded). Given the envelope/artifactId concerns raised inadeRpcServer.ts, tests here would catch regressions.🤖 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 1040 - 1103, Add test coverage in createAdeRpcRequestHandler/app/navigate for the new normalization paths. The current cases in adeRpcServer.test.ts only verify file path and commit sha handling, so extend them to assert that a commit or artifact target’s envelope fields (repoOwner, repoName, branch, prNumber, linearIssue) are preserved/normalized correctly through the handler. Also add artifact-specific checks: missing artifactId should reject with invalidParams, while a valid artifactId should be accepted and forwarded to appNavigationService.navigate. Use the existing app/navigate handler setup and the commit normalization assertions as the place to locate the new cases.apps/desktop/src/main/main.ts (1)
2158-2175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
resolveEnvelopelogic vs.ade-cli/bootstrap.ts.This
resolveEnvelopecallback (repo lookup → lane branch → PR number) is duplicated almost verbatim inapps/ade-cli/src/bootstrap.ts'screateLinearChatLinkPublisherwiring. Consider extracting a shared helper (e.g. in a module both runtimes import) that takes{ githubService, laneService, prService }accessors and returns theresolveEnvelopefunction, to avoid drift between the two nearly-identical implementations.🤖 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/main.ts` around lines 2158 - 2175, The resolveEnvelope implementation in createLinearChatLinkPublisher is duplicated with the same repo/lane/PR lookup flow used in bootstrap wiring, so extract that logic into a shared helper that both runtimes can reuse. Move the repoOwner/repoName, lane branch, and prNumber resolution into a common function that accepts the service accessors (githubService, laneServiceRef, prServiceRef) and returns the resolveEnvelope callback, then update the current publisher setup to call it instead of inlining the logic.apps/desktop/src/main/services/cto/linearLaneCardService.ts (1)
57-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate repeated envelope-building logic.
The
envelope: { repoOwner, repoName, ...(branch ? {...} : {}), ...(prNumber ? {...} : {}) }construction is duplicated verbatim inbuildCardUrl,buildLinearLaneInitialComment, andbuildLinearChatSessionAttachment. Since the envelope shape is expected to keep growing (repo/branch/pr/linear per the PR description), extracting a single helper reduces the risk of the three copies drifting apart.♻️ Proposed helper extraction
+function buildRepoEnvelope(args: { + repoOwner?: string | null; + repoName?: string | null; + branch?: string | null; + prNumber?: number | null; +}): { repoOwner: string; repoName: string; branch?: string; prNumber?: number } | undefined { + if (!args.repoOwner || !args.repoName) return undefined; + return { + repoOwner: args.repoOwner, + repoName: args.repoName, + ...(args.branch ? { branch: args.branch } : {}), + ...(args.prNumber ? { prNumber: args.prNumber } : {}), + }; +}Then each call site simplifies to, e.g.:
- envelope: { - repoOwner: args.repoOwner, - repoName: args.repoName, - branch: args.branch, - ...(args.prNumber ? { prNumber: args.prNumber } : {}), - }, + envelope: buildRepoEnvelope(args)!,Also applies to: 148-171, 216-245
🤖 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/cto/linearLaneCardService.ts` around lines 57 - 83, Consolidate the repeated lane envelope construction used in buildCardUrl, buildLinearLaneInitialComment, and buildLinearChatSessionAttachment by extracting a shared helper that returns the envelope object from repoOwner, repoName, branch, prNumber, and any future fields. Update each call site to use that helper so the envelope shape stays consistent and changes only need to be made in one place.apps/desktop/src/shared/githubRemote.ts (1)
1-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a dedicated test file.
This module underpins cross-machine repo matching (
resolveActiveProjectRepo/resolvePortableFallbackin App.tsx,repoProjectResolver.ts), but nogithubRemote.test.tsis included. Worth covering scp-like vs HTTPS forms, trailing slashes,.gitsuffix stripping, and case-insensitive equality.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/shared/githubRemote.ts` around lines 1 - 49, Add a dedicated test file for the githubRemote helpers because parseGithubRemoteUrl, parsePathParts, stripGitSuffix, and githubRepoSlugsEqual are relied on by repo matching flows like resolveActiveProjectRepo and resolvePortableFallback. Cover scp-like and HTTPS GitHub URLs, trailing slashes, .git suffix stripping, invalid/multi-segment paths, and case-insensitive owner/repo comparisons so the matching behavior is locked down.apps/desktop/src/renderer/components/app/App.tsx (1)
962-971: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the local
findForRepointersection
window.ade.projectalready declaresfindForRepoinpreload/global.d.ts, so this cast duplicates the shared IPC contract and can drift. Use the preload type directly.🤖 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/App.tsx` around lines 962 - 971, Remove the local `findForRepo` intersection around `window.ade.project` in `App.tsx` and rely on the shared preload type already declared for `window.ade.project` in `preload/global.d.ts`. Update the `projectApi` usage and the `findForRepo` call in this block to use the existing typed IPC contract directly, keeping the `App` component aligned with the canonical definition.Source: Coding guidelines
🤖 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/desktop/src/main/services/deeplinks/protocolHandler.test.ts`:
- Around line 160-217: The two “project-scoped deeplink dispatch contract” tests
are re-implementing the routing logic inline instead of exercising production
code, so they do not validate the real sync:ios dispatch path. Refactor these
cases to call the actual dispatch-selection code from protocolHandler.ts/main.ts
(exporting the relevant helper such as selectWindowForProjectNavigation or the
real dispatchSyncDeeplink path if needed) and keep the mocks around that
function, so the assertions cover the genuine project-root-vs-focused-window
decision rather than a local if/else.
In `@apps/desktop/src/renderer/components/app/App.tsx`:
- Around line 1004-1020: The file-target handling in App.tsx still dispatches a
repo-relative path when neither laneById(target.laneId) nor project?.rootPath
provides a root, but files.openExternalPath expects an absolute path. Update the
target.kind === "file" branch in App so it only builds and navigates with
externalPath when a valid absolute root is available; otherwise return early
before calling navigate. Use the existing symbols laneById, project?.rootPath,
localPath, and navigate to locate and adjust this guard.
In `@apps/ios/ADE/Views/Lanes/LaneDeeplinkHelpers.swift`:
- Around line 42-69: The branchLink() helper is passing prNumber through
directly, which allows pr=0 to be appended and then rejected by the HTTPS
router. Update branchLink() to normalize non-positive prNumber values to nil
before calling appendQuery or httpsOpenURL, matching the behavior used in
envelope(), so only nil or positive PR values are emitted.
In `@apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift`:
- Around line 461-470: The branch-link copy flow in LaneDetailScreen’s copy
action uses LaneDeeplinkHelpers.branchLink with pr.githubPrNumber without
validating it first, so a zero PR number can generate an unusable link. Guard
pr.githubPrNumber before building the URL, and if it is not a positive value,
skip copying the branch link and avoid showing the success haptic/notice;
otherwise proceed with UIPasteboard.general.string and the existing success
path.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/app/InboundDeeplinkModal.tsx`:
- Around line 390-412: The early return in onConfirm leaves busy set to true if
branchTarget.prNumber is missing, which can trap the modal in a disabled state.
Update the onConfirm flow in InboundDeeplinkModal so this guard mirrors the
existing branch-only failure path by resetting busy before returning, or move
the validation ahead of setBusy(true) if that better fits the control flow. Keep
the fix local to onConfirm and preserve the current error handling pattern used
with setError and setBusy.
In `@apps/desktop/src/renderer/components/files/v2/FilesWorkbench.tsx`:
- Around line 170-176: Capture the requested line at the time
`pendingWorkspaceOpen` is created and carry it through the async external-open
flow so `FilesWorkbench` does not re-read the live `externalOpenLine` prop after
`openExternalPathRequest` resolves. Update the state shape used by
`pendingWorkspaceOpen` and the follow-up effect(s) around the external-open
handling to use that stored line when opening/revealing the file, preventing
overlapping deeplinks from applying the wrong line to the wrong file.
---
Nitpick comments:
In `@apps/ade-cli/src/adeRpcServer.test.ts`:
- Around line 1040-1103: Add test coverage in
createAdeRpcRequestHandler/app/navigate for the new normalization paths. The
current cases in adeRpcServer.test.ts only verify file path and commit sha
handling, so extend them to assert that a commit or artifact target’s envelope
fields (repoOwner, repoName, branch, prNumber, linearIssue) are
preserved/normalized correctly through the handler. Also add artifact-specific
checks: missing artifactId should reject with invalidParams, while a valid
artifactId should be accepted and forwarded to appNavigationService.navigate.
Use the existing app/navigate handler setup and the commit normalization
assertions as the place to locate the new cases.
In `@apps/desktop/src/main/main.ts`:
- Around line 2158-2175: The resolveEnvelope implementation in
createLinearChatLinkPublisher is duplicated with the same repo/lane/PR lookup
flow used in bootstrap wiring, so extract that logic into a shared helper that
both runtimes can reuse. Move the repoOwner/repoName, lane branch, and prNumber
resolution into a common function that accepts the service accessors
(githubService, laneServiceRef, prServiceRef) and returns the resolveEnvelope
callback, then update the current publisher setup to call it instead of inlining
the logic.
In `@apps/desktop/src/main/services/cto/linearLaneCardService.ts`:
- Around line 57-83: Consolidate the repeated lane envelope construction used in
buildCardUrl, buildLinearLaneInitialComment, and
buildLinearChatSessionAttachment by extracting a shared helper that returns the
envelope object from repoOwner, repoName, branch, prNumber, and any future
fields. Update each call site to use that helper so the envelope shape stays
consistent and changes only need to be made in one place.
In `@apps/desktop/src/renderer/components/app/App.tsx`:
- Around line 962-971: Remove the local `findForRepo` intersection around
`window.ade.project` in `App.tsx` and rely on the shared preload type already
declared for `window.ade.project` in `preload/global.d.ts`. Update the
`projectApi` usage and the `findForRepo` call in this block to use the existing
typed IPC contract directly, keeping the `App` component aligned with the
canonical definition.
In `@apps/desktop/src/shared/githubRemote.ts`:
- Around line 1-49: Add a dedicated test file for the githubRemote helpers
because parseGithubRemoteUrl, parsePathParts, stripGitSuffix, and
githubRepoSlugsEqual are relied on by repo matching flows like
resolveActiveProjectRepo and resolvePortableFallback. Cover scp-like and HTTPS
GitHub URLs, trailing slashes, .git suffix stripping, invalid/multi-segment
paths, and case-insensitive owner/repo comparisons so the matching behavior is
locked down.
In `@apps/web/api/open.ts`:
- Around line 79-82: The positiveInteger helper logic is duplicated between the
API route and OpenPage.tsx, so extract the shared parsing behavior into a small
reusable module under apps/web/src and have both the open.ts handler and the
OpenPage positiveInteger/nonNegativeInteger helpers import it. Keep the existing
function semantics the same, but centralize the implementation so future changes
only happen in one place and the API and SPA stay aligned.
🪄 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: a989cfdd-7c60-4d61-92fd-36d7b4cd8d46
⛔ Files ignored due to path filters (3)
docs/features/deeplinks/README.mdis excluded by!docs/**docs/features/search/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**
📒 Files selected for processing (63)
apps/ade-cli/README.mdapps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.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/tuiClient/deeplinkRow.tsapps/desktop/resources/ade-cli-help.txtapps/desktop/resources/agent-skills/ade-deeplinks/SKILL.mdapps/desktop/src/main/main.tsapps/desktop/src/main/services/cto/linearLaneCardService.test.tsapps/desktop/src/main/services/cto/linearLaneCardService.tsapps/desktop/src/main/services/deeplinks/projectNavigationDispatch.test.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/projects/repoProjectResolver.test.tsapps/desktop/src/main/services/projects/repoProjectResolver.tsapps/desktop/src/main/services/search/searchIndexDb.tsapps/desktop/src/main/services/search/searchService.test.tsapps/desktop/src/main/services/search/searchService.tsapps/desktop/src/main/services/search/searchServiceWiring.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/App.tsxapps/desktop/src/renderer/components/app/CommandPalette.tsxapps/desktop/src/renderer/components/app/InboundDeeplinkModal.test.tsxapps/desktop/src/renderer/components/app/InboundDeeplinkModal.tsxapps/desktop/src/renderer/components/app/commandPaletteSearch.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/files/FilesTab.tsxapps/desktop/src/renderer/components/files/v2/FilesWorkbench.tsxapps/desktop/src/renderer/components/lanes/LaneContextMenu.tsxapps/desktop/src/renderer/components/terminals/TerminalView.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/pendingSessionAnchors.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/shared/deeplinks.test.tsapps/desktop/src/shared/deeplinks.tsapps/desktop/src/shared/githubRemote.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/core.tsapps/ios/ADE/ADE.entitlementsapps/ios/ADE/App/ADEApp.swiftapps/ios/ADE/App/DeepLinkRouter.swiftapps/ios/ADE/App/DeepLinkURLParsing.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Deeplinks/SendToMacCard.swiftapps/ios/ADE/Views/Lanes/LaneDeeplinkHelpers.swiftapps/ios/ADE/Views/Lanes/LaneDetailScreen.swiftapps/ios/ADE/Views/PRs/PrDetailScreen.swiftapps/ios/ADE/Views/Work/WorkChatHeaderAndMessageViews.swiftapps/ios/ADE/Views/Work/WorkRootComponents.swiftapps/ios/ADE/Views/Work/WorkRootScreen+Actions.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView+Actions.swiftapps/ios/ADE/Views/Work/WorkStatusAndFormattingHelpers.swiftapps/web/api/open.tsapps/web/public/.well-known/apple-app-site-associationapps/web/src/app/pages/OpenPage.tsxapps/web/vercel.json
💤 Files with no reviewable changes (1)
- apps/desktop/src/main/services/deeplinks/projectNavigationDispatch.test.ts
Comment on lines
+160
to
+217
|
|
||
| /** | ||
| * Regression guard for iOS sync deeplink routing: navigation must target the | ||
| * sync-host project window, not the globally focused window. | ||
| */ | ||
| describe("project-scoped deeplink dispatch contract", () => { | ||
| it("prefers project-root dispatch over focused-window dispatch for sync:ios", () => { | ||
| const focusedDispatch = vi.fn(); | ||
| const projectDispatch = vi.fn(); | ||
| let dispatchAppNavigationForProjectRoot: | ||
| | ((targetProjectRoot: string, request: AppNavigationRequest) => void) | ||
| | null = projectDispatch; | ||
|
|
||
| const projectRoot = "/projects/beta"; | ||
| const request: AppNavigationRequest = { | ||
| target: { kind: "lane", laneId: "lane-in-beta" }, | ||
| source: "deeplink:sync:ios", | ||
| }; | ||
|
|
||
| const dispatchSyncDeeplink = (req: AppNavigationRequest) => { | ||
| if (dispatchAppNavigationForProjectRoot) { | ||
| dispatchAppNavigationForProjectRoot(projectRoot, req); | ||
| return; | ||
| } | ||
| focusedDispatch(req); | ||
| }; | ||
|
|
||
| dispatchSyncDeeplink(request); | ||
|
|
||
| expect(projectDispatch).toHaveBeenCalledWith(projectRoot, request); | ||
| expect(focusedDispatch).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("falls back to focused-window dispatch when project dispatch is unavailable", () => { | ||
| const focusedDispatch = vi.fn(); | ||
| const getProjectDispatch = (): | ||
| | ((targetProjectRoot: string, request: AppNavigationRequest) => void) | ||
| | null => null; | ||
|
|
||
| const request: AppNavigationRequest = { | ||
| target: { kind: "lane", laneId: "lane-1" }, | ||
| source: "deeplink:sync:ios", | ||
| }; | ||
|
|
||
| const dispatchSyncDeeplink = (req: AppNavigationRequest) => { | ||
| const projectDispatch = getProjectDispatch(); | ||
| if (projectDispatch) { | ||
| projectDispatch("/projects/beta", req); | ||
| return; | ||
| } | ||
| focusedDispatch(req); | ||
| }; | ||
|
|
||
| dispatchSyncDeeplink(request); | ||
|
|
||
| expect(focusedDispatch).toHaveBeenCalledWith(request); | ||
| }); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
These two "dispatch contract" tests don't exercise any production code.
dispatchSyncDeeplink in both tests is a function defined entirely inline in the test body — it isn't imported from protocolHandler.ts, main.ts, or anywhere else. The tests only assert that this locally-reimplemented if/else calls the mock it's given, which is tautologically true regardless of what the real sync:ios dispatch routing does in production. The docstring at Line 161-164 calls this a "regression guard," but a bug introduced in the actual dispatchAppNavigationForProjectRoot/focused-dispatch selection logic elsewhere would not be caught here. Only the third test (Line 218-254) provides real coverage, since it calls the actual selectWindowForProjectNavigation.
Consider importing and exercising the real dispatch-selection function (exporting it for testability if needed) instead of re-implementing the branch under test.
🤖 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.test.ts` around
lines 160 - 217, The two “project-scoped deeplink dispatch contract” tests are
re-implementing the routing logic inline instead of exercising production code,
so they do not validate the real sync:ios dispatch path. Refactor these cases to
call the actual dispatch-selection code from protocolHandler.ts/main.ts
(exporting the relevant helper such as selectWindowForProjectNavigation or the
real dispatchSyncDeeplink path if needed) and keep the mocks around that
function, so the assertions cover the genuine project-root-vs-focused-window
decision rather than a local if/else.
Comment on lines
+42
to
+69
| static func branchLink( | ||
| repoOwner: String, | ||
| repoName: String, | ||
| branch: String, | ||
| prNumber: Int? = nil, | ||
| form: ADEDeeplinkForm = .https | ||
| ) -> String { | ||
| switch form { | ||
| case .ade: | ||
| let base = [ | ||
| "ade://repo", | ||
| ADEDeepLinkURLParsing.encodedPathSegment(repoOwner), | ||
| ADEDeepLinkURLParsing.encodedPathSegment(repoName), | ||
| "branch", | ||
| ADEDeepLinkURLParsing.encodedPath(branch), | ||
| ].joined(separator: "/") | ||
| return appendQuery(to: base, items: [("pr", prNumber.map(String.init))]) | ||
| case .https: | ||
| return httpsOpenURL( | ||
| items: [ | ||
| ("type", "branch"), | ||
| ("repo", "\(repoOwner)/\(repoName)"), | ||
| ("branch", branch), | ||
| ("pr", prNumber.map(String.init)), | ||
| ] | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first
echo "== LaneDeeplinkHelpers.swift =="
ast-grep outline apps/ios/ADE/Views/Lanes/LaneDeeplinkHelpers.swift --view expanded || true
echo
echo "== LaneDetailScreen.swift =="
ast-grep outline apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift --view expanded || true
echo
echo "== PrDetailScreen.swift =="
ast-grep outline apps/ios/ADE/Views/Prs/PrDetailScreen.swift --view expanded || true
echo
echo "== DeepLinkRouter.swift =="
ast-grep outline apps/ios/ADE/DeepLinks/DeepLinkRouter.swift --view expanded || true
# Read the relevant slices with line numbers
echo
echo "== LaneDeeplinkHelpers.swift (relevant lines) =="
sed -n '1,220p' apps/ios/ADE/Views/Lanes/LaneDeeplinkHelpers.swift | cat -n
echo
echo "== LaneDetailScreen.swift (relevant lines) =="
sed -n '1,260p' apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift | cat -n
echo
echo "== PrDetailScreen.swift (relevant lines) =="
sed -n '1,260p' apps/ios/ADE/Views/Prs/PrDetailScreen.swift | cat -n
echo
echo "== DeepLinkRouter.swift (relevant lines) =="
sed -n '1,260p' apps/ios/ADE/DeepLinks/DeepLinkRouter.swift | cat -n
# Search for the specific branchLink callers and any pr>0 guards
echo
echo "== branchLink usages and positive PR guards =="
rg -n "branchLink\\(|positiveInteger\\(|githubPrNumber|pr=0|prNumber > 0|canAutoMapCurrentPr|hasADELink" apps/ios/ADE -SRepository: arul28/ADE
Length of output: 18349
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate DeepLinkRouter.swift and PR detail screen =="
fd -a "DeepLinkRouter.swift" apps/ios || true
fd -a "PrDetailScreen.swift" apps/ios || true
fd -a "PRDetailScreen.swift" apps/ios || true
fd -a "*Pr*Detail*Screen*.swift" apps/ios || true
echo
echo "== Search for branchLink caller and zero-sentinel guards =="
rg -n "branchLink\\(|githubPrNumber|prNumber > 0|> 0\\s*\\)|hasADELink|canAutoMapCurrentPr|copyBranchLink" apps/ios/ADE -S
echo
echo "== Inspect the branch-link caller in LaneDetailScreen.swift =="
# show a tighter slice around copyBranchLink and related helpers if present
rg -n -A40 -B20 "copyBranchLink|copyLaneLink|githubPrNumber" apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift -S
echo
echo "== Inspect DeepLinkRouter.swift once path is known =="
router_path="$(fd -a "DeepLinkRouter.swift" apps/ios | head -n 1 || true)"
if [ -n "${router_path}" ]; then
sed -n '1,260p' "$router_path" | cat -n
fi
echo
echo "== Inspect PR detail screen once path is known =="
pr_path="$(fd -a "PrDetailScreen.swift" apps/ios | head -n 1 || true)"
if [ -z "${pr_path}" ]; then
pr_path="$(fd -a "PRDetailScreen.swift" apps/ios | head -n 1 || true)"
fi
if [ -n "${pr_path}" ]; then
sed -n '1,260p' "$pr_path" | cat -n
fiRepository: arul28/ADE
Length of output: 40949
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== LaneDetailScreen.swift: lanePullRequests population and PR-number uses =="
rg -n -A25 -B25 "lanePullRequests|githubPrNumber|copyBranchLink|loadDetail|PullRequestListItem" apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift -S
echo
echo "== PrDetailScreen.swift: zero-sentinel / positive-number guards =="
sed -n '300,380p' apps/ios/ADE/Views/PRs/PrDetailScreen.swift | cat -n
echo
sed -n '730,760p' apps/ios/ADE/Views/PRs/PrDetailScreen.swift | cat -n
echo
echo "== PullRequestListItem definition =="
rg -n -A40 -B10 "struct PullRequestListItem|let githubPrNumber: Int|var githubPrNumber: Int" apps/ios/ADE -S
echo
echo "== Places that intentionally use githubPrNumber == 0 =="
rg -n "githubPrNumber: 0|githubPrNumber \\?\\? 0|> 0" apps/ios/ADE/Views apps/ios/ADE/Models apps/ios/ADE/Services -SRepository: arul28/ADE
Length of output: 50366
Guard prNumber before appending it here. branchLink() can emit pr=0, but the HTTPS router rejects branch links unless pr is nil or positive, so the link is dropped instead of opening. Normalize non-positive values to nil like envelope() does.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ios/ADE/Views/Lanes/LaneDeeplinkHelpers.swift` around lines 42 - 69, The
branchLink() helper is passing prNumber through directly, which allows pr=0 to
be appended and then rejected by the HTTPS router. Update branchLink() to
normalize non-positive prNumber values to nil before calling appendQuery or
httpsOpenURL, matching the behavior used in envelope(), so only nil or positive
PR values are emitted.
Comment on lines
461
to
470
| if let pr = lanePullRequests.first { | ||
| let url = LaneDeeplinkHelpers.branchLink(repoOwner: pr.repoOwner, repoName: pr.repoName, branch: branch) | ||
| let url = LaneDeeplinkHelpers.branchLink( | ||
| repoOwner: pr.repoOwner, | ||
| repoName: pr.repoName, | ||
| branch: branch, | ||
| prNumber: pr.githubPrNumber | ||
| ) | ||
| UIPasteboard.general.string = url | ||
| ADEHaptics.success() | ||
| copiedLinkNotice = "Copied branch link" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== LaneDetailScreen.swift (relevant slice) ==\n'
sed -n '430,500p' apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift
printf '\n== LaneDeeplinkHelpers.swift (full or relevant slice) ==\n'
fd -a 'LaneDeeplinkHelpers.swift' apps/ios || true
if [ -f "apps/ios/ADE/Views/Lanes/LaneDeeplinkHelpers.swift" ]; then
sed -n '1,240p' apps/ios/ADE/Views/Lanes/LaneDeeplinkHelpers.swift
fi
printf '\n== Search for githubPrNumber guards and branchLink callers ==\n'
rg -n "githubPrNumber\s*>\s*0|githubPrNumber\s*==\s*0|branchLink\(" apps/ios/ADE -S
printf '\n== DeepLinkRouter route guards (search) ==\n'
rg -n "positiveInteger|pr=|branch.*route|routeHttpsOpenURL|branchLink" apps/ios/ADE -SRepository: arul28/ADE
Length of output: 9494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== DeepLinkURLParsing.swift (relevant slice) ==\n'
sed -n '1,140p' apps/ios/ADE/App/DeepLinkURLParsing.swift
printf '\n== DeepLinkRouter.swift (relevant slice) ==\n'
sed -n '130,190p' apps/ios/ADE/App/DeepLinkRouter.swiftRepository: arul28/ADE
Length of output: 8025
Guard pr.githubPrNumber before building the branch link
If githubPrNumber can be 0 here, pr=0 is rejected by ADEDeepLinkURLParsing.positiveInteger, so the copied branch link looks successful but never routes on the receiving device.
🐛 Proposed fix
if let pr = lanePullRequests.first {
let url = LaneDeeplinkHelpers.branchLink(
repoOwner: pr.repoOwner,
repoName: pr.repoName,
branch: branch,
- prNumber: pr.githubPrNumber
+ prNumber: pr.githubPrNumber > 0 ? pr.githubPrNumber : nil
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let pr = lanePullRequests.first { | |
| let url = LaneDeeplinkHelpers.branchLink(repoOwner: pr.repoOwner, repoName: pr.repoName, branch: branch) | |
| let url = LaneDeeplinkHelpers.branchLink( | |
| repoOwner: pr.repoOwner, | |
| repoName: pr.repoName, | |
| branch: branch, | |
| prNumber: pr.githubPrNumber | |
| ) | |
| UIPasteboard.general.string = url | |
| ADEHaptics.success() | |
| copiedLinkNotice = "Copied branch link" | |
| if let pr = lanePullRequests.first { | |
| let url = LaneDeeplinkHelpers.branchLink( | |
| repoOwner: pr.repoOwner, | |
| repoName: pr.repoName, | |
| branch: branch, | |
| prNumber: pr.githubPrNumber > 0 ? pr.githubPrNumber : nil | |
| ) | |
| UIPasteboard.general.string = url | |
| ADEHaptics.success() | |
| copiedLinkNotice = "Copied branch link" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ios/ADE/Views/Lanes/LaneDetailScreen.swift` around lines 461 - 470, The
branch-link copy flow in LaneDetailScreen’s copy action uses
LaneDeeplinkHelpers.branchLink with pr.githubPrNumber without validating it
first, so a zero PR number can generate an unusable link. Guard
pr.githubPrNumber before building the URL, and if it is not a positive value,
skip copying the branch link and avoid showing the success haptic/notice;
otherwise proceed with UIPasteboard.general.string and the existing success
path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcf0a74858
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function resolveGitConfigPath(rootPath: string): string | null { | ||
| const gitDir = resolveGitDir(rootPath); | ||
| return gitDir ? path.join(gitDir, "config") : null; |
There was a problem hiding this comment.
Repo matching fails for normal git worktree checkouts: their .git file points at .git/worktrees/<name>, while origin remains in the common dir referenced by commondir, so <gitdir>/config usually does not exist. In that case readGithubOriginSlug returns null and project.findForRepo cannot offer to switch to a recent project opened from a worktree. Follow commondir or fall back to the common config when resolving the config path.
Useful? React with 👍 / 👎.
| ]); | ||
| } | ||
| case "file": { | ||
| const base = `ade://file/${encodePath(target.path)}`; |
There was a problem hiding this comment.
Here the landing page trusts the raw path query when building the custom URL. For a malformed HTTPS link such as /open?type=file&path=src/../../package.json, the shared parser would reject the HTTPS target, but this page emits ade://file/src/../../package.json; WHATWG URL normalization in the desktop parser turns that into package.json and opens a different repo file. Apply the same repo-relative path validation before returning a file target or URL.
Useful? React with 👍 / 👎.
arul28
added a commit
that referenced
this pull request
Jul 7, 2026
… chrome visibility - Web-mode flag (window.__adeWebClient) + isWebClientMode() helper - TabNav: only Work/Lanes/Files/PRs on web (hide tool tabs + Settings gear) - OnboardingBootstrap: skip the desktop welcome/onboarding modal on web (it was a full-viewport overlay covering the shell's machine-title strip) - AppShell root fills the shell content area (h-full) on web instead of overflowing the viewport (h-screen) beneath the strip - WebShell strip raised above the App's viewport-fixed layers - webRoutes: support the #713 grammar (file/commit/artifact targets + session event/offset anchors) as web routes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Jul 7, 2026
… chrome visibility - Web-mode flag (window.__adeWebClient) + isWebClientMode() helper - TabNav: only Work/Lanes/Files/PRs on web (hide tool tabs + Settings gear) - OnboardingBootstrap: skip the desktop welcome/onboarding modal on web (it was a full-viewport overlay covering the shell's machine-title strip) - AppShell root fills the shell content area (h-full) on web instead of overflowing the viewport (h-screen) beneath the strip - WebShell strip raised above the App's viewport-fixed layers - webRoutes: support the #713 grammar (file/commit/artifact targets + session event/offset anchors) as web routes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Jul 7, 2026
…r sync) (#719) * Web client foundation: browser sync client, window.ade adapter, shell, host commands, deploy - Browser sync protocol client (WebCrypto DPoP w/ raw->DER, PIN pairing, IndexedDB multi-machine env store, relay/loopback endpoint racing, changeset-batch invalidation without a local DB) - Second Vite build target (webclient.html -> dist/web-client) reusing the desktop renderer App; deployed to Cloudflare Pages (ade-web-client.pages.dev) with strict CSP _headers + SPA _redirects - window.ade web adapter: full four-tab surface over remote commands, file/terminal/chat sub-protocols, invalidation-driven events, proxy safety net - Web shell: pair flow (/pair hash payload + PIN), machine switcher, project picker, open-in-desktop handoff, /open deeplink resolver over shared grammar - Host: 45 web-parity remote commands (work/chat/git/terminal/prs/rebase/ history/github/projectConfig/ai/orchestration) + queueLandingService injection fix; "browser" SyncPeerDeviceType preserved end-to-end - Desktop: Settings > Sync web client section (pair QR/link, paired browser list w/ revoke); Open in web menu items - CLI/TUI: ade link --web, web URL rows; shared webClientUrl helpers - Marketing: Web client nav links + /pair hash-forward page Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: web client feature README + cross-links Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Web client: restrict to four tabs, hide desktop onboarding, fix shell chrome visibility - Web-mode flag (window.__adeWebClient) + isWebClientMode() helper - TabNav: only Work/Lanes/Files/PRs on web (hide tool tabs + Settings gear) - OnboardingBootstrap: skip the desktop welcome/onboarding modal on web (it was a full-viewport overlay covering the shell's machine-title strip) - AppShell root fills the shell content area (h-full) on web instead of overflowing the viewport (h-screen) beneath the strip - WebShell strip raised above the App's viewport-fixed layers - webRoutes: support the #713 grammar (file/commit/artifact targets + session event/offset anchors) as web routes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * web client: dev server serves the web client entry on every route The webclient dev server's Vite root also contains the desktop renderer's index.html, which Vite served at / and /work — so hitting those routes showed the desktop app + browserMock (all tabs, mock data), not the web client. Add a dev middleware that rewrites HTML navigation requests to webclient.html, so dev matches the production SPA fallback and any route loads the web client. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Settings web client: add copy-code button + show pairing PIN alongside copy-link Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * web client: bulletproof machine strip + hide Electron-only top-bar controls - WebShell strip is now a fixed top overlay (max z-index) with the App pinned in a fixed layer directly below it, so the machine/project chrome can't be covered by the reused App's own viewport-fixed layers regardless of its internal layout. - TopBar hides new-window, add-project-window, and the auto-updater on web (no OS windows / no desktop updater there) so the web client shows no dead buttons. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * web client: fix Files tab crash — return host's structured file results directly file_request resolves with the host's structured result (workspaces/tree arrays, and a SyncFileBlob only for readFile/readArtifact). The adapter wrongly treated every result as a blob and JSON.parsed a non-existent .content, so listWorkspaces/ listTree/searchText/readFileRange silently returned empty and the Files tab crashed downstream on undefined paths ('Cannot read properties of undefined (reading endsWith)'). Return the structured result directly; keep blob decoding only for readFile/readArtifact. Fake sync client in tests now models the real structured-result contract, plus a regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Mobile: pair a browser from the phone (sync.getWebPairingInfo + iOS sheet) Host: new runtime remote command sync.getWebPairingInfo returns the web-client pairing URL + current PIN to an already-paired, DPoP-authenticated controller (a paired device already has full command access, so this adds no pre-auth exposure). Built from the same connect info the desktop settings use. iOS: 'Pair a browser' row on the Settings pairing page opens a sheet with a QR, the app.ade-app.dev pairing link (copy/share), the pairing code (copy), a no-PIN callout, and an off-network relay warning. Descriptor-gated so older hosts degrade gracefully. Verified with a full simulator build (BUILD SUCCEEDED). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * web client: bind peer to per-project host on enter; mark projects as existing Two related fixes for empty tabs + a struck-through (missing) project tab: - enterProject now always sends project_switch to bind the peer onto the project's per-project sync host, even when the catalog already reports the project active. The brain-level fallback handler that serves the catalog does NOT serve file_request or project-scoped commands, so without the switch Files/Lanes/Work/PRs all silently resolved empty (Files: 'No files workspace'; Work: 'No sessions'). Mirrors the iOS flow. - adapter project.listRecent now sets RecentProjectSummary.exists (true for an available catalog entry). It was omitted → undefined → the renderer treated the project as missing and struck through its tab. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * web client: redesign the welcome/pair screens - Real ADE logo in the header (replaces the tiny purple square + 'ADE' text) - Welcome title 'ADE Web'; cleaner caption - Full-width pair button - Instructions now cover BOTH pairing sources: ADE desktop (Settings > Sync > Web client) and the ADE iPhone app (Settings > Pair a browser) - 'Install the desktop app' link to the download page for users without a machine Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * web client: welcome/pair layout — page-corner logo, larger card Logo pinned to the page's top-left (bigger), larger title, and a wider/roomier card, so the pre-app screens fill the page instead of a small centered column. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * web client: re-resolve GitHub status on reconnect instead of forcing disconnected The adapter emitted a hardcoded githubDisconnectedStatus() on every 'connected' event, which pinned the 'GitHub not connected' banner even when the machine had GitHub connected. Re-fetch the real github.getStatus on (re)connect so the banner reflects the machine and self-heals across reconnects/host upgrades. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * web client: only switch project when not already active (faster entry) The always-switch (added on a since-disproven theory) forced a disconnect + reconnect on every project entry; the host serves the peer's bound project without it. Skip the switch when the project is already active to cut startup latency. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ade cli: add 'ade sync web' — print the web client pairing link + code Builds the app.ade-app.dev/pair link client-side from sync.getStatus (works against any brain, no new RPC), alongside the machine's 6-digit code — the CLI analogue of the desktop Settings > Sync > Web client section and the iOS 'Pair a browser' screen. Text + JSON output. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ade sync web: --open/--no-clipboard flags, clipboard-by-default; document in Agent Skills - --open opens the pairing link in the default browser (reuses the ade open OS opener); --no-clipboard disables the default copy-to-clipboard (text mode). - Documented 'ade sync web' + 'ade link --web' in the ade-deeplinks and ade-cli-control-plane Agent Skills so agents and ade help surface them. - Fixed ade link --web round-trip validation for the app.ade-app.dev form. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * desktop welcome modal: polish layout — larger card, cleaner play button, bigger surface tiles - Card 940px -> 1080px wide, taller max height. - Video poster: radial vignette + halo ring behind the play button so it reads cleanly against a busy poster; wider video<->QR gap. - 'One app, every surface' tiles enlarged (box 200->250, frames 262->300, phone scaled proportionally) so the product screenshots are legible. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * web client: default help chips/tooltips/hints off + hide Publish button - Help preferences (help chips, detailed hover tooltips, 'Did you know' hints) default OFF in the browser web client (they're onboarding aids that clutter an already-oriented user); an explicit toggle is still honored, desktop unchanged. - Hide the 'Publish to GitHub' pill on web: origin detection can't reliably resolve there (and it's desktop-oriented chrome), so it was showing even for repos that already have a GitHub origin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * web client: fix terminal streaming — stop scrollback fetch from killing the live stream Terminal preview / readTranscriptTail fetched scrollback by subscribing to the session, which shares the single per-session terminal-subscription slot with the live data stream — so it clobbered the live handlers and then unsubscribed, stopping live output (input then corrupted the render until a reload). It also re-shipped a 512KB snapshot each time (the slowness). - captureSnapshot now uses requestTerminalHistory (request/response) — never touches the live subscription. - Live subscribe requests 1KB instead of 512KB (scrollback comes from the history/preview path, delivered exactly once — not via pty.onData). - Regression test: preview/tail no longer unsubscribe or clobber the live stream, and later terminal_data still reaches the terminal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Revert: keep the Publish button visible on web Publishing to GitHub is a legit web-client feature; keep it. Its wrong appearance for repos that already have an origin is the old-brain GitHub detection gap (resolves once the brain advertises github.getRemoteStatus), not a reason to hide the feature. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * web client: wire github.publishCurrentProject so Publish works from the browser Adds the github.publishCurrentProject remote command (delegates to the host githubService, mirroring github.getStatus) + adapter mapping, replacing the 'unsupported' stub. On hosts that don't advertise it yet the descriptor-gated call falls back cleanly. Publishing a repo from the web client now creates it and pushes, same as desktop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * web client: sync hardening from incident RCA (good-citizen against a shared brain) The brain-hang incident (RCA) exonerated the web client but surfaced client-side amplification vectors. Harden them: - Client request timeout 30s -> 65s (>= the brain's 60s message timeout) so the client waits for the brain's own result instead of abandoning a still-running command and inviting a duplicate behind it. - Bound the auth-failure reconnect: after 5 consecutive failures (attributed or not) stop looping, emit auth_failed, and purge the stale saved env — a stale secret against an old brain that doesn't echo host.deviceId can no longer retry forever. - visibilitychange reconnect respects current backoff + debounces focus dials. - Dedupe the project-entry catalog fetch (shell passes it to the adapter; in-flight catalog requests coalesce). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * web client: don't auto-retry github.publishCurrentProject (mutation) Quality review: publishCurrentProject inherited the adapter call() default idempotent=true, so a recoverable transport error could retry a repo create+push. Publishing is a mutation — mark it non-idempotent like the other mutations (sendToSession, saveTempAttachment, killDroidWorker). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs+cli parity: web client, pair-a-browser, remote-commands, ade sync web/link --web /test parity passes: - docs: web-client README source map (webClientMode + reused-renderer chrome gating, shell pickers, iOS pair sheet); ios-companion 'Pair a browser' subsection; sync device_type += browser + web-client participant; remote-commands filled in (terminal/rebase/history/github/ai/... families + sync.getWebPairingInfo); deeplinks --web + TUI buildWebClientUrlForRow. - cli README: ade sync web (--open/--no-clipboard) + ade link --web entries. Docs validate (167 files). TUI + iOS parity needed no changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix web adapter refresh contracts * Tighten web sync review edges * Guard web stream subscriptions during project switch --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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
Deeplinks overhaul: fixes the three deferred items from #709, adds portable link envelopes with a graceful resolution ladder, adds commit/artifact/file targets, and brings iOS to parity (Universal Links included).
What Changed
PR #709 deferred items
search.querywith a supplied-but-all-invalidkindsarray now returns empty results instead of silently broadening to the default kind set (mirrors the inlinekind:bogusreject), with regression tests.ade open "ade://session/<id>?event=<seq>"scrolls the chat to the anchored message (matched by persisted envelope sequence, ordinal fallback with full history, bounded older-history paging + highlight);?offset=<bytes>best-effort-positions replayed terminal scrollback by byte fraction. The ⌘K palette passes the same anchors.filetarget in the shared parser/builder (ade://file/<path>?line=<n>&lane=<uuid>), dispatched lane-aware into the Files editor with real line reveal; search emits the canonical form.Portable envelopes + resolution ladder
repo/branch/pr/linearquery params, populated at mint time and parsed leniently. On open: local → open exactly; repo matches another known project (newproject.findForRepoIPC over recents' git configs, no subprocess, mtime-cached) → switch-project card; foreign machine → fallback-only card (create lane from branch / open PR or commit on GitHub / open Linear issue).InboundDeeplinkModalgeneralized — no second modal, and deliberately no request-access path.ade searchresults all mint envelope-carrying links.New targets + CLI
commit(owning lane's git detail locally, GitHub commit URL as foreign fallback) andartifact(proof/history surface, local-only).ade linkmintsfile/commit/artifactwith envelope enrichment and a--no-envelopeflag, and refuses to mint links the shared parser would reject.iOS + web
applinks:ade-app.deventitlement,NSUserActivityrouting, AASA served fromapps/web/.well-known(/open*+/pair*only, JSON content-type). https session links now navigate locally on the phone; new targets route to the envelope-aware Send-to-Mac card with GitHub-PR/Linear fallback actions; lane/session/PR copy-link minting with envelopes.Security
app/navigateRPC boundary and renderer dispatch (the RPC path bypasses URL parsing — traversal is rejected in both places, with regression tests).Validation
Desktop + ade-cli typechecks; deeplinks/search/modal/resolver/chat-list/RPC suites green (300+ affected tests); unit-main shard 1/8 smoke (773 tests); iOS verified with direct
xcodebuild(BUILD SUCCEEDED);apps/webbuild green; docs validator green. Dual-review quality pass applied 7 fixes (1 High: RPC path traversal) before push.Risks
offsetanchors are best-effort: capped replay tails and live terminals skip positioning by design.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Greptile Summary
This PR overhauls ADE deeplinks across CLI, desktop, iOS, and web. The main changes are:
Confidence Score: 4/5
The relay tunnel ownership path still needs a fix before merging.
Non-host project scopes can still satisfy the relay host guard, and a shared tunnel client can stay connected while pointing at no active sync port.
apps/ade-cli/src/bootstrap.ts
What T-Rex did
Important Files Changed
Comments Outside Diff (2)
General comment
src/adeRpcServer.test.tswith 4 spawn_agent-related failures. The failing tests expectruntime.ptyService.createto be called withcommandand/orargsfields for Claude/Codex invocation flags, but the actual call only includes fields such aslaneId,rows,cols,env,toolType,tracked,title, and a combinedstartupCommand. This breaks the PR's stated RPC boundary validation subset.command/argsPTY options intostartupCommand, while the RPC boundary expectations still require or verify the structured fields.commandandargsfields in theptyService.createoptions for spawn_agent sessions, or intentionally update the RPC contract and tests to assert the newstartupCommand-only behavior. Ifcommand/argsare part of the expected PTY/session boundary contract, prefer restoring them while keepingstartupCommandfor display or shell launch purposes.apps/ade-cli/src/bootstrap.ts, line 1309-1316 (link)When a project scope inherits
sharedSyncListener, this guard treats it as a relay host even when that scope does not own the sync server. The shared tunnel client can then keep a control connection whosegetSyncPort()returnsnullor the wrong listener, so off-LAN phones reach the relay but cannot bridge into the active sync host.Artifacts
Repro: focused harness source for non-host relay start with null sync port
Repro: harness output showing tunnel start invoked while getSyncPort is null
Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "ship: sweep stale-state dispatch class —..." | Re-trigger Greptile