Fix connection reliability and polish mobile, desktop, and web connection UX#835
Merged
Conversation
Backend: relay bridge validation now runs proactively on control-socket open and listener bind (fixes the bootstrap deadlock that blocked PIN-less phone adoption); live LAN addresses keep their canonical route kind instead of degrading to 'saved'; machine names publish with a Beta/Alpha channel suffix; project registry gains catalogVisibility with registration intent, desktop recents mirroring, and a migration that hides legacy auto-registered rows from the mobile catalog and roster. iOS: switch fast-path over the last-proven route, early entry on hello_ok with parallel hydration, bounded MRU scope pre-warm, and the Hub catalog stays mounted during switches; hero-minimal first-open and real-logo sign-in; quick-connect cards on the no-machine home; Settings reordered with a top-3 + See-all machines list and inline per-row connection errors; PIN keypad grid alignment, auto-submit, wrong-code shake, and a "no pairing code set" state fed by the QR payload and a typed pin_not_set failure; push diagnostics collapsed with an honest disabled state for notifications; voice-input toggle removed (always available); analytics default-on with all existing budgets and a coarse event set for sign-in/adoption/pairing/ quick-connect; Chats moved to a top-bar icon. Desktop: Clerk avatar hosts allowlisted in the renderer CSP with a monogram fallback; This-Mac card gets a chip, laptop icon, and live reachability/ version lines; Connect-a-phone row removed (Phone tab owns it); machine menu portaled above cards; per-tab connected dots. Web: masthead web-client primary button; /pair is now an app-first fallback that reads nothing from the fragment; hosted client signed-out state is sign-in-only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- The mobile roster and project catalog always include the active sync-host project even when its registry record is system-visibility (restores the documented host-inclusion invariant; regression-tested). - Registry visibility writes skip the file rewrite when nothing changes. - iOS machine rows consolidate into one MachineRowView (row/card surfaces) with unified device-symbol and status-copy helpers; identical sameSyncHost comparators hoisted to one function; quick-connect cluster extracted to HubQuickConnect.swift (HubComponents back under 1k lines). - Stale pairing QRs no longer dead-end: "Enter code anyway" returns to the keypad when a code was set after the QR was captured. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Interactive `ade code` attaches and explicit `ade code remote --project` selections register as recent/cli-explicit so actively-used projects appear in the mobile catalog; non-interactive probes stay system (regression tests added; add() never demotes). - `ade projects list --text` shows the visibility column; ade-cli README documents the v2 registry fields. - Feature docs updated for proactive relay validation, pinConfigured QR hint, visibility-filtered catalog/roster with host inclusion, channel suffixes, switch fast-path/prewarm, new iOS view files, and the app-first /pair fallback (validator green, 193 files). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThe pull request adds project catalog metadata and migration, account-directory health reporting, credential/session notifications, sync bridge validation, pairing payload updates, iOS analytics and connection flows, desktop connection UI changes, and web pairing and navigation updates. ChangesProject catalog and sync infrastructure
Desktop surfaces
iOS experience
Web entry points
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
arul28
changed the title
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
July 16, 2026 17:33
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Migration adds from desktop recents now register recent/desktop instead of system/runtime-auto (Greptile P1), pinned by a real-registry test; the runtimeBridge clone-auth tests assert the new truthful connect payload shape. 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: a649f6163f
ℹ️ 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".
…scovery on macOS 15 A signed-in Mac could silently fail to register in the account directory with nothing surfacing why. The publisher now records a typed outcome for every attempt and exposes it as routeHealth.accountDirectory (state + skipReason + directoryOrigin + lastHttpStatus), shown in `ade sync status` and the desktop This-Mac panel — no more invisible skips. Publishing fires immediately on sign-in instead of waiting for the 30s heartbeat, and the brain resolves the same directory origin as the desktop account bridge so a prod/dev issuer mismatch can't cause a silent 401. Also: add NSLocalNetworkUsageDescription + NSBonjourServices to the packaged macOS app so it can browse `_ade-sync._tcp` under macOS 15 Local Network privacy (a fresh install could advertise but not discover peers). Review fixes: prewarm skips hidden system projects; desktop QR generation threads the pinConfigured hint so scanned codes open the no-PIN state directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
ADE/apps/ade-cli/src/services/projects/projectScope.ts
Lines 201 to 202 in 16a2aa5
Filter system projects before selecting the sync host
When ade serve starts without ADE_PROJECT_ROOT, this still chooses the most-recent row across the entire registry. This commit adds hidden system rows and updates their lastOpenedAt from automatic CLI registrations, so a background command run in an arbitrary repo can become the active phone-sync host after restart and leak that hidden project back into the phone catalog via the host inclusion path. Prefer listRecent() here, falling back only when there are no recent projects.
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
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/services/projects/projectScope.ts (1)
211-257: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSerialize the entire sync-host transition.
syncHostTransitionDepthonly observes overlap. Two concurrent calls can read the same previous host, then independently set and enable different targets, leaving multiple scopes host-enabled whilesyncHostProjectIdnames only one. Protect the deactivate→activate→restore transaction with a mutex or promise queue and test overlapping switches.🤖 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/services/projects/projectScope.ts` around lines 211 - 257, The entire switchSyncHost transition must be serialized rather than only tracked with syncHostTransitionDepth. Add or reuse a mutex/promise queue around the complete deactivate, activate, failure-restore, and state-update flow in switchSyncHost, ensuring overlapping calls cannot enable multiple sync hosts; preserve the existing transition-depth cleanup and add coverage for concurrent switches.
🧹 Nitpick comments (1)
apps/ade-cli/src/services/sync/rosterBuilder.ts (1)
489-504: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
.slice()call.The
Array.prototype.filter()method returns a new array. Chaining.slice()before.sort()is unnecessary because mutating the filtered array with.sort()will not affect the original array returned by.list().♻️ Proposed refactor
export async function buildRosterSnapshot(args: BuildRosterSnapshotArgs): Promise<SyncRosterProject[]> { const records = args.projectRegistry .list() .filter( (record) => record.catalogVisibility === "recent" || record.projectId === args.hostProjectId, ) - .slice() .sort((left, right) => right.lastOpenedAt - left.lastOpenedAt);🤖 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/services/sync/rosterBuilder.ts` around lines 489 - 504, Remove the redundant .slice() call from the records construction in buildRosterSnapshot, keeping the existing filter criteria and subsequent most-recently-opened-first sort unchanged.
🤖 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/services/account/accountMachinePublisherService.ts`:
- Around line 521-543: The account machine publisher wrapper must expose the
runtime sync state so publish can distinguish disabled sync. Update the
createAccountMachinePublisherService call and its surrounding options to provide
the isSyncEnabled callback, returning the CLI’s existing syncEnabled value while
preserving the other callbacks and configuration.
In `@apps/ade-cli/src/services/credentials/credentialStore.ts`:
- Around line 575-583: Update the unreadable-store catch path in the credential
deserialization method to continue returning an empty object for read-only
access, but throw the decryption/deserialization failure when called with
allowRewrite: true. Ensure setSync, deleteSync, and updateSync cannot rewrite
the store after an unreadable read, while preserving lastReadState =
"unreadable".
In `@apps/ade-cli/src/services/projects/projectRegistry.ts`:
- Around line 315-360: Update the registry-loading logic around the isLegacy
determination to treat only the explicitly supported legacy version as legacy;
preserve version 2 handling and reject unknown versions such as version 3 before
mapping or rewriting projects. Ensure the rejection path does not call
write(file), preventing future-version metadata from being downgraded or
discarded.
In `@apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts`:
- Around line 969-971: Update the cache-hit condition in the project lookup flow
to require that the cached registration also matches the requested
registrationSource, in addition to system catalog visibility. Ensure
ensureProject sends a new registration when the source differs, while preserving
the existing cached return when both visibility and registrationSource match.
In `@apps/desktop/src/renderer/components/account/AccountPage.tsx`:
- Around line 459-475: Update the menu flow around openMenu, closeMenu, and the
portaled menu rendering to manage keyboard focus: retain the triggering element,
focus the menu item when the menu opens, close the menu on Escape, and restore
focus to the trigger on close. Apply the same behavior to the related menu
instances identified by the existing open/close handlers and rendered menu
blocks.
In `@apps/desktop/src/renderer/components/app/ConnectionsPanel.tsx`:
- Around line 328-341: Update the tab’s accessible name in the activeTabs
rendering path to include “active connection” when activeTabs[key] is true,
while keeping the decorative status dot’s aria-hidden attribute unchanged.
Locate the tab label or accessible-name element surrounding this indicator and
preserve the existing name for inactive tabs.
- Around line 235-263: Update the snapshot handling in the ConnectionsPanel
useEffect so both the initial getConnectionSnapshot() result and
onConnectionSnapshotChanged events only update state when their updatedAt is at
least as recent as the current connectionSnapshot. Preserve cancellation and
unsubscribe behavior, and ensure an older initial response cannot overwrite a
newer event.
In `@apps/desktop/src/renderer/components/settings/accountDirectorySummary.ts`:
- Around line 7-11: Update the accountSignedIn handling in
accountDirectorySummary to condition the nearby-device message on
pairingPinConfigured: retain the connection guidance when a pairing code exists,
but use concrete setup guidance such as “Set a pairing code so nearby devices
can connect” when it does not. Keep healthy false for the signed-out state.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 4859-4870: Clear the pairing failure state by resetting
lastPairingErrorCode and lastPairingFailure before calling
ensureDatabaseReady(). Keep these resets applied for every pairing attempt,
including when database initialization fails, so SettingsPinSheet cannot reuse
stale failure data.
In `@apps/ios/ADE/Views/Account/MobileAccessGateView.swift`:
- Around line 41-52: Update the sign-in button in MobileAccessGateView so
accountLoading displays a visible loading indicator and the unconfigured-account
state displays an explicit unavailable message. Preserve the existing enabled
button label and action for configured, non-loading accounts, while keeping the
control disabled whenever accountConfigured is false or accountLoading is true.
- Around line 16-82: Wrap the access-gate content in MobileAccessGateView’s
outer layout with a vertical ScrollView so the action buttons and
accountConnectionError remain reachable on compact screens and large Dynamic
Type. Preserve the existing spacing, styling, and centered appearance by
applying a minimum viewport-height container inside the scroll view rather than
relying on the fixed VStack height.
In `@apps/ios/ADE/Views/Hub/HubComponents.swift`:
- Around line 834-838: Make the no-machine Hub layout adaptive by constraining
the quick-connect content so primary reconnect and Settings controls remain
visible: in apps/ios/ADE/Views/Hub/HubComponents.swift:834-838, make the
no-machine content scrollable or give HubQuickConnectSection a bounded region;
in apps/ios/ADE/Views/Hub/HubQuickConnect.swift:45-64, cap the number of inline
cards and add navigation to the full machine list.
In `@apps/ios/ADE/Views/Hub/HubQuickConnect.swift`:
- Around line 22-36: Update the HubQuickConnect target-building flow around
onlineAccountMachines and onlinePairedHosts to coalesce records that represent
the same machine using deviceId/hostIdentity before rendering cards. Merge
matching account and saved records while combining their reachability state, and
ensure hasTargets and card generation consume the deduplicated collection rather
than independently filtered arrays.
In `@apps/ios/ADE/Views/Settings/ConnectionSettingsView.swift`:
- Around line 608-646: In ConnectionSettingsView.entries, replace the separate
account and saved-host passes with canonical targets grouped by stable machine
identity, merging account metadata with saved-host reachability and route
information before creating one Entry per machine. Preserve current-selection
detection across both identities. In HubQuickConnect.swift lines 22-36, consume
the same canonical target collection so account-paired machines are rendered
only once; both sites must use the shared merged representation rather than
independently deduplicating records.
In `@apps/ios/ADE/Views/Settings/SettingsPairingSection.swift`:
- Around line 228-234: Update sameSyncHost in
apps/ios/ADE/Views/Settings/SettingsPairingSection.swift (lines 228-234) to stop
using unconditional hostName equality; match only stable host identity, merge
key, or uniquely disambiguated address data. In
apps/ios/ADE/Views/Settings/ConnectionSettingsView.swift (lines 634-637), retain
host-name fallback only when no stable identity is available.
In `@apps/ios/ADE/Views/Settings/SettingsPinSheet.swift`:
- Around line 170-174: Update the cancel action in SettingsPinSheet’s Button
handler to reset isSubmitting before calling
syncService.disconnect(clearCredentials: false) and dismiss(). Preserve the
existing conditional disconnect behavior while ensuring post-pairing completion
handling is suppressed after cancellation.
In `@apps/ios/ADE/Views/Settings/SettingsPushDeliverySection.swift`:
- Around line 96-129: Update enableNotificationsControl to derive the permission
affordance from pushService.permissionStatus, which is refreshed by
refreshNotificationSettings(), rather than snapshot.permissionStatus. Refresh
notification settings before the relay/status request in the .task sequence, and
use the current service status for the denied-state label and Settings action
while preserving the existing pairing and enable flows.
In `@apps/web/src/app/pages/PairPage.tsx`:
- Around line 44-46: Update the instructional copy in the PairPage component to
tell users to scan the pairing code again after installing ADE, while preserving
the existing installation guidance.
In `@apps/web/src/components/editorial/Masthead.tsx`:
- Around line 40-68: Update the Masthead navigation actions so narrow mobile
viewports do not overflow: below the sm breakpoint, use a compact label for the
web-client CTA while preserving the full “Open web client” label at larger
widths, and ensure the iOS, web-client, and Mac actions remain visible within
the existing non-wrapping navigation. Anchor the change to the web-client link
using LINKS.webClient and MARKETING_FEATURES.OPEN_WEB_CLIENT.
---
Outside diff comments:
In `@apps/ade-cli/src/services/projects/projectScope.ts`:
- Around line 211-257: The entire switchSyncHost transition must be serialized
rather than only tracked with syncHostTransitionDepth. Add or reuse a
mutex/promise queue around the complete deactivate, activate, failure-restore,
and state-update flow in switchSyncHost, ensuring overlapping calls cannot
enable multiple sync hosts; preserve the existing transition-depth cleanup and
add coverage for concurrent switches.
---
Nitpick comments:
In `@apps/ade-cli/src/services/sync/rosterBuilder.ts`:
- Around line 489-504: Remove the redundant .slice() call from the records
construction in buildRosterSnapshot, keeping the existing filter criteria and
subsequent most-recently-opened-first sort unchanged.
🪄 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: 2c7c4b70-bfc9-47d0-ae93-314b8b852547
⛔ Files ignored due to path filters (6)
apps/ios/ADE.xcodeproj/project.pbxprojis excluded by!**/*.xcodeproj/project.pbxprojdocs/features/remote-runtime/README.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/**docs/features/web-client/README.mdis excluded by!docs/**docs/logging.mdis excluded by!docs/**
📒 Files selected for processing (87)
apps/ade-cli/README.mdapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/multiProjectRpcServer.test.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/ade-cli/src/services/account/accountAuthService.test.tsapps/ade-cli/src/services/account/accountAuthService.tsapps/ade-cli/src/services/account/accountMachineDirectoryService.test.tsapps/ade-cli/src/services/account/accountMachinePublisherService.test.tsapps/ade-cli/src/services/account/accountMachinePublisherService.tsapps/ade-cli/src/services/account/sharedAccountAuthService.test.tsapps/ade-cli/src/services/account/sharedAccountAuthService.tsapps/ade-cli/src/services/credentials/credentialStore.test.tsapps/ade-cli/src/services/credentials/credentialStore.tsapps/ade-cli/src/services/projects/projectRegistry.test.tsapps/ade-cli/src/services/projects/projectRegistry.tsapps/ade-cli/src/services/projects/projectScope.test.tsapps/ade-cli/src/services/projects/projectScope.tsapps/ade-cli/src/services/sync/rosterBuilder.test.tsapps/ade-cli/src/services/sync/rosterBuilder.tsapps/ade-cli/src/services/sync/sharedSyncListener.tsapps/ade-cli/src/services/sync/syncPairingConnectInfo.test.tsapps/ade-cli/src/services/sync/syncPairingConnectInfo.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/services/sync/syncService.tsapps/ade-cli/src/services/sync/syncTunnelClientService.test.tsapps/ade-cli/src/services/sync/syncTunnelClientService.tsapps/ade-cli/src/tuiClient/__tests__/appPolling.test.tsxapps/ade-cli/src/tuiClient/__tests__/connection.test.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/connection.tsapps/ade-cli/src/tuiClient/remoteLauncher.tsapps/desktop/package.jsonapps/desktop/src/main/main.tsapps/desktop/src/main/rendererCsp.test.tsapps/desktop/src/main/rendererCsp.tsapps/desktop/src/main/services/account/accountBridge.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/ipc/runtimeBridge.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/remoteRuntime/remoteBootstrap.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionService.tsapps/desktop/src/main/services/runtime/machineStateMigration.test.tsapps/desktop/src/main/services/runtime/machineStateMigration.tsapps/desktop/src/renderer/components/account/AccountPage.test.tsxapps/desktop/src/renderer/components/account/AccountPage.tsxapps/desktop/src/renderer/components/app/ConnectionsPanel.test.tsxapps/desktop/src/renderer/components/app/ConnectionsPanel.tsxapps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsxapps/desktop/src/renderer/components/settings/SyncDevicesSection.tsxapps/desktop/src/renderer/components/settings/accountDirectorySummary.tsapps/desktop/src/renderer/webclient/shell/MachinePicker.tsxapps/desktop/src/renderer/webclient/shell/__tests__/MachinePicker.test.tsxapps/desktop/src/shared/pairingQr.test.tsapps/desktop/src/shared/pairingQr.tsapps/desktop/src/shared/types/remoteRuntime.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/PrivacyInfo.xcprivacyapps/ios/ADE/Services/AccountService.swiftapps/ios/ADE/Services/PairingQrPayload.swiftapps/ios/ADE/Services/ProductAnalytics.swiftapps/ios/ADE/Services/PushNotificationService.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Account/AccountConnectionsSection.swiftapps/ios/ADE/Views/Account/AccountSignInView.swiftapps/ios/ADE/Views/Account/MobileAccessGateView.swiftapps/ios/ADE/Views/Components/DictationMicButton.swiftapps/ios/ADE/Views/Components/MachineRowView.swiftapps/ios/ADE/Views/Hub/HubComponents.swiftapps/ios/ADE/Views/Hub/HubQuickConnect.swiftapps/ios/ADE/Views/Hub/HubScreen.swiftapps/ios/ADE/Views/Settings/ConnectionSettingsView.swiftapps/ios/ADE/Views/Settings/SettingsAnalyticsSection.swiftapps/ios/ADE/Views/Settings/SettingsPairingSection.swiftapps/ios/ADE/Views/Settings/SettingsPinSheet.swiftapps/ios/ADE/Views/Settings/SettingsPushDeliverySection.swiftapps/ios/ADE/Views/Settings/SettingsVoiceInputSection.swiftapps/ios/ADEClip/PrivacyInfo.xcprivacyapps/ios/ADETests/ADETests.swiftapps/ios/ADETests/PairingAndDpopTests.swiftapps/ios/ADETests/ProductAnalyticsPolicyTests.swiftapps/ios/ADEWidgets/PrivacyInfo.xcprivacyapps/web/src/app/pages/PairPage.tsxapps/web/src/components/editorial/Masthead.tsx
💤 Files with no reviewable changes (2)
- apps/ios/ADE/Views/Settings/SettingsVoiceInputSection.swift
- apps/ios/ADE/Views/Settings/SettingsAnalyticsSection.swift
Token/account reliability: - getAccessToken now decides refresh from the token's real JWT exp claim, not the stored expiresAt bookkeeping — a token whose metadata claims validity but whose JWT is expired is refreshed (or cleanly reported expired) instead of being shipped and 401'd. OAuth expires_in is only a fallback. - On invalid_grant, re-read account.session.v1 and retry once if another process rotated the refresh token (desktop app ↔ brain rotation race). - Capture the directory's 401 reason (token expired / invalid issuer / invalid audience, bounded 512B) into routeHealth.accountDirectory so failures are named in `ade sync status` and the desktop panel. CI: credentialStore change-notification test is deterministic via a checkForChangesNow() hook instead of real-time polling. Review feedback (CodeRabbit/Greptile): registry rejects unknown versions and never downgrades on cache hit; unreadable credential stores stay fail-closed on read but throw on mutation instead of rewriting; publisher takes isSyncEnabled; connection snapshots are timestamp-ordered across Run/TopBar/CommandPalette/ RemoteTargets; portaled account menu manages focus; iOS quick-connect/access-gate/ pin-sheet/push-permission/host-matching quick wins; web pair-page and masthead copy. Analytics stays default-on with no opt-out by product decision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Addressed this iteration's review threads. Two intentional non-changes:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37361af6de
ℹ️ 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".
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 (1)
apps/desktop/src/renderer/components/app/CommandPalette.tsx (1)
399-418: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear the previous connection snapshot when reopening.
Because fetch failures are ignored and state survives closing, a reopened palette can indefinitely expose machines that are no longer connected.
Proposed fix
useEffect(() => { if (!open) return; + setRemoteSnapshot(null); const remoteRuntime = window.ade.remoteRuntime;🤖 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/CommandPalette.tsx` around lines 399 - 418, Reset the previously applied remote connection snapshot when the command palette is reopened, before fetching the current snapshot in the useEffect guarded by open. Ensure failed fetches cannot leave stale machines visible, while preserving live updates and cleanup through applyRemoteSnapshot and the existing unsubscribe logic.Source: Path instructions
🤖 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/account-directory/src/directory.ts`:
- Around line 405-408: Update the authentication failure handling after
authenticate in the directory request flow to return status 503 when
authentication.reason is "authentication unavailable"; preserve the existing 401
response for all other authentication failures.
In `@apps/ade-cli/src/services/account/accountAuthService.ts`:
- Around line 1488-1490: The refresh path around buildSessionRecord must use
credentialStore.updateSync() as a compare-and-swap, persisting refreshed only
when the stored refresh token still equals refreshRecord.refreshToken. If it
differs, do not overwrite the session; restart token resolution using the latest
stored session, while preserving the existing return behavior for a successful
compare-and-swap.
In `@apps/ade-cli/src/services/credentials/credentialStore.ts`:
- Around line 173-175: Update the credential-change notification flow around the
listener callback to isolate subscriber failures so an exception cannot escape
the setInterval callback or terminate the ADE process. Match the best-effort
observer handling used by notifySignedIn(), while preserving the existing
credential state update and notification behavior.
In `@apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsx`:
- Around line 189-198: Ensure latestConnectionSnapshotUpdatedAtRef stays
synchronized whenever local connect, disconnect, or settings code creates and
applies a newer connection snapshot, not only in the IPC snapshot handler.
Centralize snapshot application if appropriate, or update the ref alongside each
local setConnectionSnapshot call so delayed older IPC snapshots are still
rejected.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/app/CommandPalette.tsx`:
- Around line 399-418: Reset the previously applied remote connection snapshot
when the command palette is reopened, before fetching the current snapshot in
the useEffect guarded by open. Ensure failed fetches cannot leave stale machines
visible, while preserving live updates and cleanup through applyRemoteSnapshot
and the existing unsubscribe logic.
🪄 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: 9db6769b-5ad3-4ac9-89b5-7f7d0423868d
⛔ Files ignored due to path filters (3)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/agents/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**
📒 Files selected for processing (43)
apps/account-directory/src/directory.tsapps/account-directory/test/directory.test.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/multiProjectRpcServer.test.tsapps/ade-cli/src/services/account/accountAuthService.test.tsapps/ade-cli/src/services/account/accountAuthService.tsapps/ade-cli/src/services/account/accountMachineDirectoryService.test.tsapps/ade-cli/src/services/account/accountMachinePublisherService.test.tsapps/ade-cli/src/services/account/accountMachinePublisherService.tsapps/ade-cli/src/services/credentials/credentialStore.test.tsapps/ade-cli/src/services/credentials/credentialStore.tsapps/ade-cli/src/services/projects/projectRegistry.test.tsapps/ade-cli/src/services/projects/projectRegistry.tsapps/ade-cli/src/tuiClient/__tests__/remoteLauncher.test.tsapps/ade-cli/src/tuiClient/remoteLauncher.tsapps/desktop/src/main/services/account/accountBridge.trust.test.tsapps/desktop/src/main/services/account/accountBridge.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/renderer/components/account/AccountPage.test.tsxapps/desktop/src/renderer/components/account/AccountPage.tsxapps/desktop/src/renderer/components/app/CommandPalette.tsxapps/desktop/src/renderer/components/app/ConnectionsPanel.test.tsxapps/desktop/src/renderer/components/app/ConnectionsPanel.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsxapps/desktop/src/renderer/components/run/RunPage.tsxapps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsxapps/desktop/src/renderer/components/settings/accountDirectorySummary.test.tsapps/desktop/src/renderer/components/settings/accountDirectorySummary.tsapps/desktop/src/shared/accountDirectory.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Account/MobileAccessGateView.swiftapps/ios/ADE/Views/Hub/HubComponents.swiftapps/ios/ADE/Views/Hub/HubQuickConnect.swiftapps/ios/ADE/Views/Settings/ConnectionSettingsView.swiftapps/ios/ADE/Views/Settings/SettingsPairingSection.swiftapps/ios/ADE/Views/Settings/SettingsPinSheet.swiftapps/ios/ADE/Views/Settings/SettingsPushDeliverySection.swiftapps/web/src/app/pages/PairPage.tsxapps/web/src/components/editorial/Masthead.tsx
🚧 Files skipped from review as they are similar to previous changes (23)
- apps/desktop/src/shared/types/sync.ts
- apps/desktop/src/renderer/components/settings/accountDirectorySummary.ts
- apps/ade-cli/src/tuiClient/remoteLauncher.ts
- apps/web/src/app/pages/PairPage.tsx
- apps/ios/ADE/Views/Settings/SettingsPushDeliverySection.swift
- apps/ios/ADE/Views/Hub/HubQuickConnect.swift
- apps/desktop/src/renderer/components/account/AccountPage.test.tsx
- apps/desktop/src/renderer/components/app/ConnectionsPanel.tsx
- apps/desktop/src/renderer/components/app/ConnectionsPanel.test.tsx
- apps/desktop/src/main/services/account/accountBridge.ts
- apps/web/src/components/editorial/Masthead.tsx
- apps/desktop/src/renderer/components/account/AccountPage.tsx
- apps/ade-cli/src/services/account/accountMachineDirectoryService.test.ts
- apps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsx
- apps/ade-cli/src/services/projects/projectRegistry.ts
- apps/ios/ADE/Views/Account/MobileAccessGateView.swift
- apps/ios/ADE/Views/Settings/ConnectionSettingsView.swift
- apps/ios/ADE/Views/Hub/HubComponents.swift
- apps/ios/ADE/Views/Settings/SettingsPinSheet.swift
- apps/ade-cli/src/services/account/accountMachinePublisherService.ts
- apps/ade-cli/src/cli.ts
- apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts
- apps/ios/ADE/Services/SyncService.swift
…cher isolation - Machine-only sync.getStatus returns a complete typed SyncRoleSnapshot (localDevice, client, pairing/runtime fields, accountDirectory health). - Directory Worker returns 503 when Clerk auth is unconfigured; clients map it to unavailable, never auth_expired. - Session persistence compares refreshToken/obtainedAt before writing so a concurrent refresh or sign-out wins and resolution restarts from disk. - Credential change listeners are isolated; one failing subscriber cannot break siblings or the watcher. - Remote-target snapshot ordering ref advances on local connect/disconnect/ auto-connect updates; stale IPC snapshots rejected. 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: def1a3828d
ℹ️ 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".
An offline account-directory row no longer claims a machine identity ahead of a currently-discovered saved pairing for the same Mac — the reachable record wins, so Settings can always reconnect to a live paired machine. 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
Dogfooding follow-up to #834: fixes the two live connection bugs and polishes every connection surface.
Backend
savedrelabeling), so directory records publish real LAN endpoints.catalogVisibility(recent/system) with registration intent on every path, desktop recents mirroring, one-time migration hiding legacy auto-registered rows, and the active sync-host project always included in catalog + roster. Interactiveade code/ade code remoteselections register as recent.iOS
pinConfiguredhint + typedpin_not_setfailure, with "Enter code anyway" for stale QRs).Desktop
Web
/pairis an app-first fallback that reads nothing from the fragment; hosted client signed-out state is sign-in-only.Testing
🤖 Generated with Claude Code
Summary by CodeRabbit
Greptile Summary
This PR updates connection reliability and connection-related UX across backend, iOS, desktop, and web. The main changes are:
/pairfallback changed to an app-first iPhone install page.Confidence Score: 4/5
Mostly safe to merge after the registry migration bug is fixed.
One contained issue affects legacy project catalog visibility. The reviewed sync, relay, pairing, and UI paths otherwise have clear guards around the changed behavior.
apps/ade-cli/src/services/projects/projectRegistry.ts
What T-Rex did
Important Files Changed
/pairfrom fragment forwarding to app-first iPhone install fallback without reading the URL payload.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant CLI as ADE CLI/Runtime participant Registry as ProjectRegistry v2 participant Listener as SharedSyncListener participant Relay as ADE Relay participant Directory as Account Directory participant Phone as iOS/Web Client CLI->>Registry: add/touch project with visibility + source CLI->>Listener: ensureListening(port candidates) Listener->>Listener: bind + assert loopback nonce Listener-->>CLI: loopback validated status CLI->>Relay: open signed control socket CLI->>Listener: validate relay bridge to local listener CLI->>Directory: publish reachable endpoints gated by route health Phone->>Directory: read machine endpoints/catalog Phone->>Relay: connect via published relay endpoint Relay->>CLI: open pipe request CLI->>Listener: bridge WebSocket with private proof Listener-->>Phone: sync hello/catalog/roster%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant CLI as ADE CLI/Runtime participant Registry as ProjectRegistry v2 participant Listener as SharedSyncListener participant Relay as ADE Relay participant Directory as Account Directory participant Phone as iOS/Web Client CLI->>Registry: add/touch project with visibility + source CLI->>Listener: ensureListening(port candidates) Listener->>Listener: bind + assert loopback nonce Listener-->>CLI: loopback validated status CLI->>Relay: open signed control socket CLI->>Listener: validate relay bridge to local listener CLI->>Directory: publish reachable endpoints gated by route health Phone->>Directory: read machine endpoints/catalog Phone->>Relay: connect via published relay endpoint Relay->>CLI: open pipe request CLI->>Listener: bridge WebSocket with private proof Listener-->>Phone: sync hello/catalog/rosterComments Outside Diff (2)
apps/desktop/src/shared/pairingQr.ts, line 164 (link)pinConfiguredis an optional additive field, but raising the parser floor toversion >= 3makes every existing v2 pairing QR from an older desktop fail before the payload is read. That breaks mixed-version pairing when the phone/web client updates before the Mac; accept the previous payload version and treat the missingpinConfiguredasundefined/nil.Artifacts
Repro: focused Vitest harness for legacy v2 pairing QR parser compatibility
Repro: verbose Vitest output showing v2 parses as null and comparable v3 parses successfully
Prompt To Fix With AI
apps/ade-cli/src/services/projects/projectRegistry.ts, line 334-337 (link)Preserve hidden legacy rows
This migration promotes every legacy v1 record whose
rootPathstill contains a.gitdirectory torecent, even when it was not in the desktop recent set. A v1 registry populated by runtime auto-registration therefore republishes old/system-only projects inlistRecent(), so the mobile catalog and roster surface rows the migration is supposed to hide.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Artifacts
Repro: focused ProjectRegistry migration script
Repro: legacy v1 registry fixture used by the script
Repro: command output showing migrated catalogVisibility and listRecent result
Prompt To Fix All With AI
Reviews (5): Last reviewed commit: "Let online saved machines override stale..." | Re-trigger Greptile