Fix mobile sync project identity aliases#584
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughAdds a "forget project" capability across the full sync stack. New ChangesProject Forget Flow and Project ID Aliases
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
changed the title
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@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: 7161f7dcbc
ℹ️ 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 closeTimer = setTimeout(() => { | ||
| void closeProjectContext(rootToClose).catch((error) => { |
There was a problem hiding this comment.
In the desktop main-process forget path, a paired phone can remove a project that is currently open in a desktop window; scheduling closeProjectContext directly disposes the DB/services and may clear activeProjectRoot, but it bypasses the closeProjectByPath bookkeeping that unbinds windowProjectRoots/tabs, emits project changes, and replaces the dormant context. In that scenario the window can remain bound to a disposed context (and active work can be torn down) after a "Remove from list" action; route this through the window-aware close path or avoid tearing down open contexts.
Useful? React with 👍 / 👎.
| rootPath: requestedRootPath || null, | ||
| }; | ||
| } | ||
| projectRegistry.remove(record.projectId); |
There was a problem hiding this comment.
In the headless ade serve provider, this only removes the registry row; any cached ProjectScope for the same project remains alive, and ProjectScopeRegistry.get() returns cached scopes before consulting the registry. If a phone removes the currently hosted project, it disappears from the catalog but its sync host and command handling can keep running until process restart; the existing projects.remove RPC path disposes the scope before removing the registry entry, and this path should do the same.
Useful? React with 👍 / 👎.
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ade-cli/src/bootstrap.ts (1)
1251-1256:⚠️ Potential issue | 🔴 CriticalDesktop sync-service wiring is missing
runtimeProjectIdparameter.The CLI now forwards
runtimeProjectId: projectId(bootstrap.ts:1251-1256), but the desktop paired-phone construction at main.ts:3590 does not pass this parameter. Without it, catalog-ID clients will lack the alias context and fall intoproject_mismatcherrors.Add
runtimeProjectId: projectIdto the desktopcreateSyncServicecall at main.ts:3590 to mirror the CLI path.🤖 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/bootstrap.ts` around lines 1251 - 1256, The desktop paired-phone construction is missing the runtimeProjectId parameter when calling createSyncService, while the CLI path correctly includes runtimeProjectId: projectId. Add the runtimeProjectId: projectId parameter to the createSyncService call in the desktop implementation to match the CLI wiring and provide the necessary alias context for catalog-ID clients to avoid project_mismatch errors.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/ade-cli/src/cli.ts`:
- Around line 13176-13177: The scopeRegistry.dispose(record.projectId) call is
being awaited immediately, which can close the host/peer before the
project_forget_result response is sent by the sync handler. Keep the
projectRegistry.remove(record.projectId) call synchronous and immediately after
it, defer the scopeRegistry.dispose(record.projectId) operation using
setImmediate or a similar mechanism to schedule it for execution after the
current event loop iteration completes, ensuring the response has time to be
queued before the scope is disposed.
- Around line 13148-13155: The current OR-based find logic in
projectRegistry.list().find() can select the wrong project when a request
carries conflicting projectId and rootPath parameters that point to different
records. Instead of using the OR condition, resolve projectId and rootPath
matches independently by performing separate find operations, then validate that
if both requestedId and requestedRootPath are provided (both have length > 0),
they must identify the same project record. If they identify different projects,
reject the operation. If they identify the same project or only one is provided,
use that matched record. This prevents stale or mobile-mixed payloads from
inadvertently operating on the wrong registered project.
In `@apps/ade-cli/src/services/sync/syncService.ts`:
- Around line 689-691: The `projectIdAliases` construction correctly populates
the alias array based on the `runtimeProjectId` parameter in the
`createSyncService` function, but the desktop socket-backed path does not pass
`runtimeProjectId` when invoking `createSyncService`, leaving `projectIdAliases`
empty. This causes phones to encounter project_mismatch errors when sending
runtime/catalog IDs. Ensure that the desktop socket-backed initialization passes
`runtimeProjectId: projectId` to the `createSyncService` call, matching the
pattern used in the CLI/headless initialization path.
In `@apps/desktop/src/main/main.ts`:
- Around line 5098-5110: The rootToForget determination logic does not validate
that requestedRoot and requestedProjectId refer to the same project when both
are provided. Before assigning to rootToForget, add a check: if both
requestedRoot and requestedProjectId are present, verify that the found recent
entry (or contextMatch) satisfies BOTH conditions simultaneously (not just one
or the other). If both identifiers are provided but they resolve to different
projects, return null to indicate an ambiguous request rather than silently
prioritizing one identifier over the other and potentially forgetting the wrong
project.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Line 1579: The performInitialHydration method reads
database.listMobileProjects() directly without filtering hidden projects, while
refreshProjectCatalog() now applies hidden-project filtering using filter {
!isProjectHidden($0) }. This inconsistency allows forgotten cached projects to
be treated as selectable during initial hydration. Apply the same filter {
!isProjectHidden($0) } pattern to the database.listMobileProjects() call in
performInitialHydration to ensure hidden projects are consistently filtered
across both methods and prevent hidden cached projects from being restored as
active during reconnect or initial hydration scenarios.
- Around line 2257-2261: The unhideProject method only removes keys returned by
hiddenKeys(for:), but since rememberHiddenProject stores both id: and root:
prefixed keys for the same project, other aliases can remain hidden. Modify
unhideProject to subtract all key variants for the project: in addition to the
keys from hiddenKeys(for:), also remove any root: prefixed keys that match the
project's root identifier, ensuring all aliases are unhidden when opening the
same project through different identifiers.
- Around line 1540-1549: When forgetting the active project in the wasActive
block, add cleanup for project-scoped live state (chat/terminal subscriptions
and Work/terminal streams) after calling setActiveProjectId(nil), before calling
refreshActiveSessionsAndSnapshot(). This cleanup should mirror the same
operations performed during a project switch to ensure stale subscriptions from
the removed project are properly cleared rather than remaining active until a
subsequent reconnect.
---
Outside diff comments:
In `@apps/ade-cli/src/bootstrap.ts`:
- Around line 1251-1256: The desktop paired-phone construction is missing the
runtimeProjectId parameter when calling createSyncService, while the CLI path
correctly includes runtimeProjectId: projectId. Add the runtimeProjectId:
projectId parameter to the createSyncService call in the desktop implementation
to match the CLI wiring and provide the necessary alias context for catalog-ID
clients to avoid project_mismatch errors.
🪄 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: 0d70ffc4-3708-4dec-99d9-5a5803429b02
⛔ Files ignored due to path filters (2)
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 (13)
apps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.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/Models/RemoteModels.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADETests/ADETests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd325cbe7a
ℹ️ 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".
| projectIdAliases: args.runtimeProjectId && args.runtimeProjectId !== args.projectId | ||
| ? [args.runtimeProjectId] | ||
| : [], |
There was a problem hiding this comment.
When the legacy desktop sync host is enabled (ADE_ENABLE_DESKTOP_SYNC_HOST=1), apps/desktop/src/main/main.ts still constructs createSyncService with only the DB-local projectId, so this new alias list remains empty. A phone whose active project id came from the machine catalog/runtime will continue sending that registry id on project-scoped command or changeset envelopes, and this host still rejects it as a project_mismatch instead of treating it as the same open project.
Useful? React with 👍 / 👎.
| if let id = normalizedProjectId(project.id) { | ||
| keys.insert("id:\(id)") | ||
| } | ||
| if let root = normalizedProjectRoot(project.rootPath) { | ||
| keys.insert("root:\(root)") | ||
| } |
There was a problem hiding this comment.
When the phone is paired with multiple machines, these hidden keys are global to the app rather than tied to the active host profile. Removing /Users/.../ADE from one machine stores root:<path> and then applyRemoteProjectCatalog filters out any later catalog row from another paired machine with the same normalized path, so a valid project on that other host disappears until the user re-opens it through the add flow.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Fixes mobile Work hydration failures when a paired phone sends the project DB id while the multi-project runtime is routing by the machine project registry id. The sync host now treats the runtime/catalog id and DB-local project id as aliases for the same open project.
What Changed
project_forget_request/resultso mobile can remove a project from the project list like desktop recents.Validation
npm --prefix apps/desktop run typecheckcd apps/desktop && npx vitest run src/main/services/sync/syncHostService.test.tsnpm --prefix apps/ade-cli run typechecknpm --prefix apps/ade-cli run testnode scripts/validate-docs.mjsADETests/ADETests/testSyncServiceForgetProjectHidesCachedAndRemoteRowsByRootADERisks
Low. The alias behavior is scoped to the current host project id and explicit aliases; mobile project removal is local-first and still sends a runtime forget request when connected.
Summary by CodeRabbit
New Features
Tests
Greptile Summary
project_forget_request/resultsupport for removing projects from mobile project lists.Confidence Score: 4/5
Mostly safe to merge after addressing the iOS host-switch catalog filtering issue.
The changes are focused and covered by targeted desktop, CLI, docs, and iOS validation, but one host-scoped state ordering issue can leave valid mobile projects hidden until another refresh.
apps/ios/ADE/Services/SyncService.swift
What T-Rex did
Comments Outside Diff (2)
apps/desktop/src/main/main.ts, line 3590-3595 (link)The alias fix is only wired through the CLI runtime path. Desktop still creates the shared phone sync service with just the DB-local
projectId, socreateSyncServicenever receives the registry/runtime id asruntimeProjectIdandhostProjectIdAliasesstays empty. In the desktop paired-phone path, a phone that sends the catalog/registry id can still getproject_mismatchorproject_not_open, which leaves the mobile hydration failure in place for this host path.Artifacts
Repro: script exercising scope resolution with empty vs populated aliases
Repro: execution output showing project_mismatch for desktop path
Prompt To Fix With AI
apps/ios/ADE/Services/SyncService.swift, line 7312-7316 (link)This applies the hello project catalog before
saveProfile(profile)switchesactiveHostProfileand reloadshiddenProjectKeysfor the connected host. If the phone previously hid/tmp/fooon host A and then connects to host B whose hello payload includes/tmp/foo, this path filters host B's catalog using host A's hidden keys.saveProfile(profile)reloads the right host-scoped keys later, but the already-filtered catalog is not rebuilt, so host B's project can stay missing from Project Home until another catalog refresh. Reload the host-scoped hidden keys before applying the incoming catalog, or re-apply the catalog after switching profiles.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "ship: address hidden project review feed..." | Re-trigger Greptile