Add mobile project actions over sync#579
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
📝 WalkthroughWalkthroughThis PR implements a "Mobile Sync Project Actions" feature enabling iOS clients to browse, open, create, and clone machine-side projects over the existing WebSocket sync protocol. It adds new protocol envelope types, expands the sync host service with project-action routing, introduces a headless brain fallback WebSocket handler, wires both desktop and CLI providers, and delivers an iOS multi-step sheet UI with supporting ChangesMobile Sync Project Actions
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot review but do not make fixes |
arul28
force-pushed
the
ade/when-remote-conenctoin-latopo-dekstop-b2b52eed
branch
from
17b47ab to
88c4557
Compare
|
@codex review |
arul28
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 88c4557189
ℹ️ 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".
| }) | ||
| : null; | ||
| let scopeRegistry: InstanceType<typeof ProjectScopeRegistry>; | ||
| sharedSyncListener?.setFallbackConnectionHandler( |
There was a problem hiding this comment.
When projectRegistry.list() is empty, runSyncHostStartupLoop calls startSyncHost, which falls through to scopeRegistry.resolveActiveSyncHost() and returns null without ever calling sharedSyncListener.ensureListening(). Registering the fallback handler here therefore does not open the sync port on a fresh/headless machine, so a phone cannot pair or use the Add project flow to create the first project. Please ensure the shared listener is bound for the fallback path even when no project scope is available.
Useful? React with 👍 / 👎.
| if mode == .open, selectedPath == nil { | ||
| selectedPath = result?.openableProjectRoot |
There was a problem hiding this comment.
If the user selected a repo, then edits or pastes a different path in the text field, the next load leaves the old selectedPath intact because this branch only assigns when it is nil. Since targetPath prefers selectedPath, tapping Open project can open the previously selected repo while the browser is showing a different folder. Reset or recompute the selection whenever a new browse result is loaded.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/cli.ts`:
- Around line 13065-13083: The function mobileProjectSummaryForHeadlessRecord
currently always returns isOpen: true even when scopeRegistry.get fails to
initialize the project runtime. Modify the catch block to track initialization
failure, and only return isOpen: true when scope initialization succeeds. You
should either set a flag when the scope fails to initialize and pass isOpen:
false in the return statement, or set isOpen: false directly in the catch block
instead of just resetting laneCount.
- Around line 13111-13116: The listProjects method in
machineProjectCatalogProvider maps all registered projects through
toMobileProjectSummary which hard-codes isAvailable: true, causing deleted or
moved projects to appear selectable. Modify the mapping logic in
projectRegistry.list().map() to check if each project record is still valid
before mapping it to a mobile project summary. If a project entry is stale
(deleted or moved), either filter it out or ensure isAvailable is set to false
in the summary to prevent unavailable projects from appearing as selectable in
the mobile catalog.
- Around line 13247-13256: The bootstrap token is currently being stored as
plaintext in a file when passed via bootstrapTokenPath to
createBrainProjectActionsSyncHandler. This needs to be refactored to use
encrypted storage instead. Locate the ensureBootstrapToken function that writes
the bootstrap token and modify it to use EncryptedFileCredentialStore (which is
already used elsewhere for PIN and paired-device secrets) rather than writing
directly to a plaintext file with only filesystem permissions. Update the
createBrainProjectActionsSyncHandler call to pass the encrypted credential store
instance instead of a direct file path for the bootstrap token, ensuring it
matches the encryption approach used for the PIN and paired-devices secrets.
- Around line 13033-13041: Remove the `as never` type cast from the
githubService parameter in the createProjectScaffoldService call within the
createHeadlessProjectScaffoldService function. Since HeadlessGitHubService is
already aliased to GithubService, the types are compatible and the cast is
unnecessary and obscures type safety. Simply pass githubService directly without
the type assertion.
In `@apps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.ts`:
- Around line 323-354: The completeProjectConnection method is being called
twice in the "project_switch_request" case handler: once in the try block (line
330) and potentially again in the catch block (lines 340-343) if result exists.
To prevent duplicate invocations and potential side effects or inconsistent
state, introduce a boolean flag that tracks whether the first
completeProjectConnection call on line 330 completed successfully. Set this flag
to true only after that call completes without throwing. Then in the catch
block, only invoke completeProjectConnection if the flag is false, ensuring it
is called at most once per request handling.
In `@apps/ade-cli/src/services/sync/syncHostService.ts`:
- Around line 2545-2547: The current code in the sendProjectCatalog call only
sends the refreshed project catalog to the requesting peer. To fix this issue,
broadcast the refreshed catalog to all connected peers after mutations from
open/create/clone project actions, not just the requester. Modify the
sendProjectCatalog call to iterate through and send the payload returned by
buildProjectCatalogPayload to all connected clients/peers, ensuring that LAN
discovery metadata and other connected phones receive the updated catalog and
remain in sync.
In `@apps/ios/ADE/App/RemoteProjectAddSheet.swift`:
- Around line 425-434: The selectedPath field retains stale values from previous
path selections when users manually edit the path. In the load() function, the
condition checking if selectedPath == nil prevents the selection from being
updated on subsequent loads. Remove the selectedPath == nil check from the
conditional so that selectedPath is always reset to the newly computed
result?.openableProjectRoot value whenever mode == .open, ensuring the selection
stays synchronized with the currently browsed path.
- Around line 71-87: The onCreate and onClone callbacks fire unguarded Task
blocks that can be triggered multiple times via rapid user taps, causing
duplicate project creation or clone attempts. Add a state property (like
isSubmitting or similar) to track whether a request is currently in flight. In
both the onCreate callback (which calls createProject) and onClone callback
(which calls cloneProject), check this flag before starting the Task, set it to
true when the operation begins, and reset it to false when the operation
completes. Additionally, pass this submission state to RemoteProjectCreateForm
and RemoteProjectCloneForm so they can disable their action buttons while the
request is running, preventing further user interaction until the current
operation finishes.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 1772-1790: The `limit` parameter in the
`browseMachineProjectDirectories` function is being passed directly to the
payload without validation, which allows negative, zero, or oversized values to
be sent to the host. Before adding the `limit` value to the payload dictionary,
clamp it to a valid range (ensure it is at least 1 and at most a reasonable
upper bound such as the default value of 80 or a suitable maximum) to prevent
invalid values from causing host-side errors or unnecessary load.
🪄 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: d7abd029-4e85-49d8-8796-f8db3d4d43cf
⛔ Files ignored due to path filters (4)
apps/ios/ADE.xcodeproj/project.pbxprojis excluded by!**/*.xcodeproj/project.pbxprojdocs/ARCHITECTURE.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**
📒 Files selected for processing (15)
apps/ade-cli/src/cli.tsapps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.tsapps/ade-cli/src/services/sync/sharedSyncListener.tsapps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncService.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/sync/syncHostService.test.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/App/ContentView.swiftapps/ios/ADE/App/RemoteProjectAddSheet.swiftapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkNewChatScreen.swiftapps/ios/ADETests/ADETests.swift
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbc3c5ee0e
ℹ️ 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".
| selectedPath = nil | ||
| path = parentPath | ||
| } |
There was a problem hiding this comment.
When the user taps “Go up”, path changes but result still describes the previous directory until the debounced browse request finishes. During that window the primary button’s targetPath is still derived from the stale result, so a quick “Open project”/“Use directory” tap can act on the previous folder while the text field shows the parent. Clear result (or disable the primary action) before updating the path.
Useful? React with 👍 / 👎.
| private var hasGitHubHost: Bool { | ||
| guard let host = URL(string: trimmedUrl)?.host?.lowercased() else { return false } | ||
| return host == "github.com" || host.hasSuffix(".github.com") |
There was a problem hiding this comment.
For the URL clone path, this validation only accepts URLs that URL(string:) gives a host, so the standard GitHub SSH form git@github.com:owner/repo.git keeps the Clone button disabled even though the host-side clone parser accepts that shape. Users with SSH remotes/keys cannot submit an otherwise valid GitHub repository from iOS; mirror the backend/Desktop validation here or defer validation to the host.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffccf6a761
ℹ️ 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".
| send(peer.ws, "project_switch_result", { | ||
| ok: false, | ||
| message: error instanceof Error ? error.message : String(error), | ||
| }, envelope.requestId); |
There was a problem hiding this comment.
When the fallback handler has already sent a successful project_switch_result, any exception from completeProjectConnection falls into this catch and sends another project_switch_result with the same request id. In that scenario iOS resolves and removes the pending request on the first response, so it treats the switch as successful while the host activation may have failed and discards the later error; guard this error send once a result has already been delivered, and just log/retry completion instead.
Useful? React with 👍 / 👎.
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f83603ad6a
ℹ️ 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".
| const activeScope = await scopeRegistry.resolveActiveSyncHost(); | ||
| if (activeScope) return activeScope; | ||
| if (sharedSyncListener) { | ||
| await sharedSyncListener.ensureListening([DEFAULT_SYNC_HOST_PORT]); |
There was a problem hiding this comment.
When no preferred or active project exists, this path still binds the shared phone-sync listener on the default port, but it bypasses the normal sync host startup path that acquires acquireSyncHostSingleton before owning that port. That lets a project-less brain squat on the WebSocket port with no singleton owner; another ADE channel/brain can then acquire the lease and still fail/retry on EADDRINUSE, leaving mobile sync unavailable until the idle brain exits. The fallback listener should acquire the same singleton lease, or avoid binding until it can own it.
Useful? React with 👍 / 👎.
1 participant
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Greptile Summary
This PR adds mobile project management capabilities over the sync channel — browse, open, create, and clone projects on the paired machine from the iOS project-home screen — and introduces a machine-wide fallback sync handler (
brainProjectActionsSyncHandler) that serves these actions even before any project host is active.RemoteProjectAddSheetSwift view covers Open / Create / Clone flows, backed by six new request/result envelope pairs wired end-to-end through both the per-projectsyncHostServiceand the new brain-level fallback handler.sharedSyncListenergains asetFallbackConnectionHandlerAPI with a grace-period suppression window so reconnecting phones still park for adoption by the next primary host during handoff, not dispatched to the fallback.workCliPermissionModestops falling back toruntimeModewhenwire.permissionModeis nil, returningnilinstead.Confidence Score: 4/5
Mostly safe to merge; the one real defect is confined to a rare failure path in the brain fallback handler's project-switch completion.
The
completionAttemptedflag is set beforecompleteProjectConnectionis awaited, making the catch-block retry guard permanently false. WhencompleteProjectConnection(i.e.scopeRegistry.switchSyncHost) throws, the phone holds a success response but the machine never activates the new project's sync host. The user ends up in a state where the selected project appears switched on the phone but produces no sync connection until they manually re-select it. All other paths — the new iOS UI, the shared-listener handoff suppression, the new desktop/headless catalog provider methods — look correct and are well-tested.apps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.ts — the project_switch_request handler's retry condition for completeProjectConnection
Important Files Changed
project_switch_requestmeanscompleteProjectConnectionis never retried on failure, leaving the machine without an active sync host for the selected project.machineProjectCatalogProviderinto a standalone object (including browse/open/create/clone/listGitHubRepos actions), attaches the brain fallback handler to the shared listener, and fixesstartSyncHostto callensureListeningwhen no active scope exists. Refactoring is clean.setFallbackConnectionHandlerAPI and afallbackSuppressedUntilMsgrace-period mechanism that prevents the fallback handler from claiming reconnecting-phone sockets during the handoff window. Logic is correct and well-tested.SyncProjectCatalogProvidertype, addsprojectActionsEnabledtohello_ok, and adds handler functions for all new project-action envelope types. New action handlers correctly broadcast catalog updates to all connected peers on success.openMobileSyncProject,createMobileSyncProject, andcloneMobileSyncProjectdesktop-side handlers, wires them into theprojectCatalogProvider. Path resolution and error handling follow the same pattern as the headless CLI path.RemoteProjectDirectoryBrowser.load()is correct;isSubmittingguards on all action methods prevent double submission;hasGitHubHostproperly validates via URL host component rather than substring search.supportsProjectActionsflag, new request helpers, and methods for all six project-action endpoints.featureEnabledhelper cleanly replaces duplicated feature-flag parsing. Stale-catalog deduplication inapplyMachineProjectActionResponseis correct.runtimeModefallback inworkCliPermissionMode, returningnilwhenwire.permissionModeis nil/empty. Intentional clean-up of implicit fallback behaviour.projectActionsfeature flag type and nine new envelope type aliases for the project-action protocol messages. Additions are additive and consistent with existing envelope patterns.Sequence Diagram
sequenceDiagram participant iOS as iOS (RemoteProjectAddSheet) participant BH as Brain Fallback Handler participant SH as syncHostService (active project) participant CP as machineProjectCatalogProvider Note over iOS,CP: Before any project selected (no active host) iOS->>BH: hello (bootstrap/paired auth) BH-->>iOS: hello_ok (projectActions.enabled: true) iOS->>BH: project_browse_request BH->>CP: browseDirectories() CP-->>BH: ProjectBrowseResult BH-->>iOS: project_browse_result iOS->>BH: project_open_request / create / clone BH->>CP: openProject / createProject / cloneProject CP-->>BH: SyncMobileProjectSummary BH-->>iOS: project_open_result + project_catalog iOS->>BH: project_switch_request BH->>CP: prepareProjectConnection() CP-->>BH: SyncProjectSwitchResultPayload BH-->>iOS: project_switch_result BH->>CP: completeProjectConnection() ← scopeRegistry.switchSyncHost Note over iOS,SH: Active project host takes over shared listener iOS->>SH: project_browse_request / open / create / clone SH->>CP: browseDirectories / openProject / createProject / cloneProject CP-->>SH: result SH-->>iOS: result + broadcast project_catalog to all peersComments Outside Diff (3)
apps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.ts, line 753-783 (link)project_switch_resultresponse aftercompleteProjectConnectionfailureWhen
prepareProjectConnectionsucceeds and thencompleteProjectConnectionthrows, the handler sends both a success response (line 759) AND an error response (line 778) for the samerequestId. The iOS resolver matches the first (success) response and discards the second, but the machine never completes the sync-host activation —scopeRegistry.switchSyncHostis retried best-effort before the error send, but if both attempts fail the phone believes it has switched projects while no project sync host is actually active. The user sees stale or empty sync state until they manually re-select the project.The catch block should not send a second
project_switch_resultwhenresultis non-null. The retry ofcompleteProjectConnectionis correct, but the fallback error send on line 778 should be guarded:if (!result) { send(...) }.Prompt To Fix With AI
apps/ios/ADE/Views/Work/WorkNewChatScreen.swift, line 3343-3349 (link)runtimeModefallback silently changes existing behaviorThe old logic returned
runtimeModeas a fallback whenwire.permissionModewasnil. The new code always returnsnilin that case. If any active provider/mode combination yields anilwire.permissionMode, callers that previously received a non-nil mode string will now receiveniland skip setting a permission mode entirely. Consider confirming that no active provider+runtimeModepair relied on theruntimeModefallback before shipping.Prompt To Fix With AI
apps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.ts, line 817-851 (link)completeProjectConnectionfailurecompletionAttemptedis set totrue(line 826) immediately beforecompleteProjectConnectionis awaited (line 827). This means whencompleteProjectConnectionthrows, we enter the catch block withresultnon-null ANDcompletionAttemptedalreadytrue, so the guardif (result && !completionAttempted)is alwaysfalse— the retry block on lines 836-843 is unreachable dead code.Consequence: when
completeProjectConnection(which callsscopeRegistry.switchSyncHost) fails, the phone has already received a successproject_switch_resultand will attempt to connect to a sync host that was never fully activated. The machine logs a warning but takes no corrective action.The fix is to set
completionAttempted = trueafter thecompleteProjectConnectioncall so the catch block's retry guard can actually fire. The existing test only checks that a second result is not sent; it does not verify thatcompleteProjectConnectionis retried on failure.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "Guard remote open project submissions" | Re-trigger Greptile