Skip to content

Fix relay reliability, web client over relay, and Connections local/remote scoping#846

Merged
arul28 merged 7 commits into
mainfrom
ade/connections-relay-fixes-0f4656c2
Jul 17, 2026
Merged

Fix relay reliability, web client over relay, and Connections local/remote scoping#846
arul28 merged 7 commits into
mainfrom
ade/connections-relay-fixes-0f4656c2

Conversation

@arul28

@arul28 arul28 commented Jul 17, 2026

Copy link
Copy Markdown
Owner

Summary

Dogfooding follow-up (v1.2.29). Fixes the relay never connecting, the web client being near-unusable over relay, and the Connections panel conflating local vs remote-bound machines.

Relay (brain-side; infra confirmed healthy — 426 host route, matching claim secret):

  • Stop tearing down the relay control socket while it's still connecting (a transient account-lease re-check or overlapping reconnect closed the in-flight socket, so it never established → no relay endpoint published → web client timed out). Sign-out/expiry still close relay; transient tolerance bounded by real token expiry.
  • Observability: capture the real close code / unexpected-response body, split lastControlOpenAt from lastBridgeValidationAt, log sync_tunnel.* events, and surface relay.skipReason in ade sync status — no more black box.
  • Confirmed-409 relay-identity rotation recovery.

Web client over relay:

  • Files tab "no files" fixed (stale bound project id + errors masked as empty).
  • Second connections-pane null-deref crash fixed.
  • Latency (client-side only, net-lower relay traffic, zero added Cloudflare cost): one cached prs.getMobileSnapshot for list reads, hot-read coalescing, no redundant retry, changeset bursts no longer trigger refetch storms.

Connections panel:

  • New sync.getLocalStatus IPC → the "This Mac" card, pairing code, and device lists always represent the physically-local machine, so a remote-bound window no longer shows the remote machine mislabeled as "This Mac" or its stale browsers. Pairing mutations held read-only while remote-bound (can't hit the wrong machine).

Cost: account-directory publish heartbeat 30s→60s (90s online window keeps margin), halving that surface's requests; relay DO hibernation unchanged. Target: ≤$5/mo Cloudflare at 30-50 users.

Testing

/quality dual-review (0 Blockers; relay-lifecycle security invariants verified — sign-out closes relay, no dual sockets, tolerance bounded by token expiry; 8 findings applied incl. 2 correctness fixes with regression tests). /test: logging gate not-applicable (no analytics changes), docs validated (194 files), parity all-clean (mobile contract-safe both skew directions, CLI formatter surfaces skipReason, TUI unaffected), CI shard 3/8 green (766), sync/account suites green.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added local sync status access, including when viewing a remotely bound project.
    • Remote-bound device views now show local-machine devices and restrict management actions.
    • Added richer relay health details, including connection errors and validation timestamps.
    • Improved project binding and switching reliability across desktop and web experiences.
  • Bug Fixes

    • Improved relay reconnection, identity recovery, lease handling, and upgrade error reporting.
    • Prevented stale data from replacing newer sync or project status.
    • Reduced redundant requests through short-lived read caching and request coalescing.
    • Adjusted heartbeat behavior to avoid unnecessary network activity.

Greptile Summary

This PR improves ADE Relay reliability, web-client behavior over relay, and local-versus-remote sync scoping. The main changes are:

  • Relay control sockets now tolerate transient account-lease refresh failures and surface richer close/error details.
  • Confirmed relay identity conflicts can rotate the machine identity and trigger account-directory republishing.
  • Web-client reads now use short project-scoped coalescing and invalidation to reduce relay traffic.
  • Connections now reads local machine status separately from routed project status.
  • iOS and web sync paths preserve full relay WebSocket URLs and updated heartbeat behavior.
  • Documentation and tests were updated for relay observability, local status, and remote-bound Connections behavior.

Confidence Score: 5/5

Safe to merge with low risk.

No accepted bugs were found in the changed paths reviewed. The highest-risk relay lifecycle and local/remote scoping paths include targeted tests and conservative guards around lease expiry, identity rotation, project cache keys, and mutation routing.

No files require special attention.

T-Rex T-Rex Logs

What T-Rex did

  • Ran the connections local status test suite and confirmed all tests passed with exit code 0.
  • Attempted to capture and render the connections UI, but the process was blocked by an invalid hook call error caused by multiple React copies during Vite dependency optimization, so no UI capture was produced.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
apps/ade-cli/src/services/sync/syncTunnelClientService.ts Improves relay lifecycle handling, control observability, bridge validation, buffered pipe forwarding, and confirmed-409 identity recovery.
apps/ade-cli/src/services/sync/syncCloudRelayStore.ts Adds relay credential normalization and serialized identity rotation under an exclusive sibling lock.
apps/desktop/src/preload/preload.ts Exposes local sync status directly through IPC while retaining routed sync behavior for project-bound operations.
apps/desktop/src/main/services/ipc/registerIpc.ts Adds local sync status IPC and keeps routed status/list-device behavior scoped through runtime routing.
apps/desktop/src/renderer/components/settings/useSyncConnections.ts Fetches local and routed sync snapshots independently and derives read-only device lists when remote-bound.
apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx Renders Connections using local-machine identity and read-only device controls while remote-bound.
apps/desktop/src/renderer/webclient/adapter/infra/commandCaller.ts Adds project-scoped read coalescing and removes immediate relay retries for recoverable command failures.
apps/desktop/src/renderer/webclient/adapter/files.ts Fixes file-list result handling, surfaces list errors, and coalesces hot list/tree reads by project.
apps/desktop/src/renderer/webclient/adapter/prs.ts Coalesces PR mobile snapshot reads and avoids replacing authoritative PR state with empty fallback on invalidation.
apps/desktop/src/renderer/webclient/sync/connection.ts Adds relay account-token proof on paired hellos and heartbeat fallback based on host-advertised intervals.
apps/ios/ADE/Services/SyncService.swift Updates iOS relay route handling and reduced-load sync paths for full WebSocket relay URLs and host-unavailable retries.
apps/desktop/src/shared/types/sync.ts Renames relay reason/success fields into skipReason plus distinct control/bridge observability timestamps.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
  participant Web as Web/Desktop Client
  participant IPC as Preload/IPC
  participant Local as Local Sync Runtime
  participant Remote as Remote-bound Runtime
  participant Relay as ADE Relay
  participant Dir as Account Directory

  Web->>IPC: sync.getLocalStatus()
  IPC->>Local: local machine sync.getStatus
  Local-->>IPC: physical Mac snapshot
  IPC-->>Web: This Mac + local devices

  Web->>IPC: routed project/status/file/PR read
  alt remote-bound window
    IPC->>Remote: project-scoped runtime action/sync
    Remote->>Relay: paired WebSocket traffic when off-LAN
    Relay-->>Remote: tunneled sync frames
    Remote-->>IPC: routed result
  else local project
    IPC->>Local: local runtime action/sync
    Local-->>IPC: local result
  end
  IPC-->>Web: scoped result

  Local->>Relay: claim + host control socket
  Relay-->>Local: control open / pipe requests
  Local->>Dir: publish reachable endpoints
  Relay-->>Local: 409 claim conflict
  Local->>Local: rotate relay identity once
  Local->>Dir: request republish after validation
Loading
%%{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 Web as Web/Desktop Client
  participant IPC as Preload/IPC
  participant Local as Local Sync Runtime
  participant Remote as Remote-bound Runtime
  participant Relay as ADE Relay
  participant Dir as Account Directory

  Web->>IPC: sync.getLocalStatus()
  IPC->>Local: local machine sync.getStatus
  Local-->>IPC: physical Mac snapshot
  IPC-->>Web: This Mac + local devices

  Web->>IPC: routed project/status/file/PR read
  alt remote-bound window
    IPC->>Remote: project-scoped runtime action/sync
    Remote->>Relay: paired WebSocket traffic when off-LAN
    Relay-->>Remote: tunneled sync frames
    Remote-->>IPC: routed result
  else local project
    IPC->>Local: local runtime action/sync
    Local-->>IPC: local result
  end
  IPC-->>Web: scoped result

  Local->>Relay: claim + host control socket
  Relay-->>Local: control open / pipe requests
  Local->>Dir: publish reachable endpoints
  Relay-->>Local: 409 claim conflict
  Local->>Local: rotate relay identity once
  Local->>Dir: request republish after validation
Loading

Reviews (3): Last reviewed commit: "Avoid returning from finally in relay-lo..." | Re-trigger Greptile

arul28 and others added 4 commits July 17, 2026 05:18
…ections scoping

Relay (brain-side; infra was healthy — 426 host route, matching claim secret):
- Stop tearing down the relay control socket while it is still connecting — a
  transient account-lease re-check or overlapping reconnect no longer closes an
  in-flight socket, and one transient lease/refresh failure is tolerated up to
  real token expiry. This is why the socket never established (lastSuccessAt
  null) and no relay endpoint was published, so the web client timed out.
- Relay observability: capture the real close code / unexpected-response status
  and body, split lastControlOpenAt from lastBridgeValidationAt, log
  sync_tunnel.* events, and surface a relay skipReason in `ade sync status`.
- Confirmed-409 recovery path to re-mint the relay identity (defense in depth).

Web client over relay:
- Fix the Files tab showing "no files": project switches could bind after
  failure, file requests used a stale active project id, and request failures
  were masked as empty — now switches validate, the bound id is explicit, and
  real failures surface.
- Fix a second connections-pane null-deref crash.
- Latency (client-side only, net-lower relay traffic, zero added Cloudflare
  cost): PR lists serve from one cached prs.getMobileSnapshot, identical hot
  reads coalesce, the redundant immediate retry is removed, and changeset
  bursts stop triggering unrelated refetch storms.

Connections panel:
- The "This Mac" card, pairing code, and device lists now always represent the
  physically-local machine via a new local-only sync status path, so a
  remote-bound window no longer shows the remote machine mislabeled as "This
  Mac" or its stale browsers. Device lists are scoped/labeled, and pairing
  mutations are held read-only while remote-bound to avoid hitting the wrong
  machine.

Cost: account-directory publish heartbeat 30s -> 60s (90s online window keeps
margin), halving that surface's request volume; relay DO hibernation unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Single-source the peer->device projection and the coalescing read-cache
  (were duplicated across the web adapter and the connections panel by the
  parallel workstreams).
- Relay routeHealth exposes one skipReason field (was a redundant
  reason+skipReason that consumers read inconsistently).
- Extract useSyncConnections from the oversized SyncDevicesSection.
- Correctness: a recoverable transient error no longer caches an empty
  fallback for 3s (same class as the files-tab-empty bug), and a failed
  getLocalStatus degrades instead of falling through to showing the remote
  machine's data as "This Mac". Both regression-tested.
- Mechanical: domain->event map, RelayClaimError param-property.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ay data model

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ct lan candidate kind

The relay observability change added a getLoopbackValidationStatus() call in
the shared sync service; the desktop test's host mock needed the method. The
pairing-address test's stale 'saved' expectation is corrected to 'lan' to match
the authoritative candidate-kind behavior (production code unchanged).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Jul 17, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
ade Ignored Ignored Preview Jul 17, 2026 11:54am

@arul28 arul28 changed the title connections-relay-fixes -> Primary Fix relay reliability, web client over relay, and Connections local/remote scoping Jul 17, 2026
@arul28

arul28 commented Jul 17, 2026

Copy link
Copy Markdown
Owner Author

@copilot review but do not make fixes

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

The pull request updates relay identity recovery and diagnostics, heartbeat fallback scheduling across CLI, desktop, and iOS clients, machine-local sync status APIs, remote-bound desktop device scoping, and web adapter caching, invalidation, project binding, and PR/file read behavior.

Relay control and identity recovery

Layer / File(s) Summary
Identity rotation and account publishing
apps/ade-cli/src/services/sync/syncCloudRelayStore.ts, apps/ade-cli/src/services/account/..., apps/ade-cli/src/bootstrap.ts
Relay identity updates are serialized with an on-disk lock, invalid credentials are regenerated, and confirmed identity rotations can trigger account-machine republishing.
Relay control lifecycle and diagnostics
apps/ade-cli/src/services/sync/syncTunnelClientService.ts, apps/ade-cli/src/services/sync/syncService.ts, apps/ade-cli/src/cli.ts, apps/ade-cli/src/multiProjectRpcServer.ts
Relay control failures, lease expiry, claim conflicts, bridge validation, reconnects, and lifecycle timestamps are tracked and exposed through sync status and readiness output.
Relay behavior coverage
apps/ade-cli/src/services/sync/*.test.ts, apps/ade-cli/src/cli.test.ts
Tests cover identity locks, claim-conflict rotation, lease failures, upgrade errors, close reasons, and expanded route-health diagnostics.

Desktop local sync scope

Layer / File(s) Summary
Local sync status API
apps/desktop/src/shared/ipc.ts, apps/desktop/src/main/services/ipc/registerIpc.ts, apps/desktop/src/preload/*
A syncGetLocalStatus IPC route and preload API retrieve machine-level sync status even when the window is remote-bound.
Scoped sync state and device selection
apps/desktop/src/renderer/components/settings/useSyncConnections.ts, apps/desktop/src/shared/types/sync.ts
The settings hook combines routed and local snapshots, derives remote-bound state, selects local peers when needed, and gates device management.
Remote-bound device UI
apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx, apps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsx
Remote-bound views display local machine context, replace pairing controls with a read-only note, and hide revoke actions.

Web adapter caching and binding

Layer / File(s) Summary
Cache, command, invalidation, and project contracts
apps/desktop/src/renderer/webclient/adapter/infra/*, apps/desktop/src/renderer/webclient/adapter/index.ts
Stable cache keys, coalesced TTL reads, command cache invalidation, domain-event mapping, and explicit project IDs are added.
Cached files, lanes, and PR reads
apps/desktop/src/renderer/webclient/adapter/files.ts, lanes.ts, prs.ts
Files, lanes, and PR reads use caching, scoped invalidation, snapshot fallbacks, and refreshed invalidation events.
Adapter surface and validation
apps/desktop/src/renderer/webclient/adapter/*.ts, apps/desktop/src/renderer/webclient/adapter/__tests__/*, apps/desktop/src/renderer/webclient/shell/WebClientRoot.tsx
Remote-runtime stubs, peer-derived sync devices, explicit project binding, switch failure handling, and adapter integration coverage are wired together.

Heartbeat fallback scheduling

Layer / File(s) Summary
CLI and desktop heartbeat schedulers
apps/ade-cli/src/services/sync/syncPeerService.ts, apps/desktop/src/renderer/webclient/sync/connection.ts
Heartbeat fallback timers use recursive timeouts, longer defaults, and inbound-message activity to avoid unnecessary client pings.
iOS silence fallback
apps/ios/ADE/Services/SyncService.swift, apps/ios/ADETests/ADETests.swift
The iOS client uses a doubled server interval and sends fallback probes only after inbound traffic has been silent long enough.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

  • arul28/ADE#816: Extends the same CLI sync route-health and doctor diagnostic plumbing.
  • arul28/ADE#827: Shares the account machine-publishing flow updated here.
  • arul28/ADE#835: Shares the sync tunnel bridge-validation path and status derivation.

Suggested labels: desktop, web, ios, docs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.06% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the PR’s main themes: relay reliability, web client behavior over relay, and Connections scoping.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ade/connections-relay-fixes-0f4656c2

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/services/sync/syncCloudRelayStore.ts`:
- Around line 201-209: Update rotateMachineIdentity and setRelayUrl to use a
shared locked read-modify-write primitive for every whole-file configuration
update, rather than locking rotations only. Ensure the primitive acquires the
rotation lock, loads the latest config while locked, applies the update, writes
the result, and releases the lock; preserve rotateMachineIdentity’s
expectedMachineKey check and return behavior.
- Around line 157-175: Update acquireRotationLock and its release logic to use
an owner token/PID in the lock file, avoiding stale-lock deletion when the
recorded owner is still alive. When releasing, read the lock contents and unlink
only if they still match this acquisition’s token; preserve locks belonging to a
replacement owner and handle missing files safely.
- Around line 129-141: Update the identity selection logic around
validMachineKey, validSecret, and mintIdentity so that if either persisted field
fails validation, both machineKey and secret come from the newly minted
credential pair. Only reuse persisted values when both fields are valid, and
preserve the existing missing-identity error check.

In `@apps/ade-cli/src/services/sync/syncTunnelClientService.ts`:
- Line 159: Replace the boolean claimed cache used by claimOnce() with cached
relay-origin and machine-key identity values, and only skip claiming when both
match the current relay URL and identity() result. Reset or update these cached
values when either changes so external store rotation and relay changes trigger
a new claim. Add a regression test covering external rotation and repeated claim
attempts with the new identity.

In
`@apps/desktop/src/renderer/webclient/adapter/__tests__/connectionsPane.test.tsx`:
- Around line 54-76: Wrap the ConnectionsPanel test body after assigning
window.ade in a try/finally block so cleanup always runs, including when
rendering or assertions fail. In finally, unmount the rendered view when
available, dispose the adapter, and restore the saved previousAde global.

In `@apps/desktop/src/renderer/webclient/adapter/infra/cacheKey.ts`:
- Around line 1-10: Update stableCacheKey so undefined, null, NaN, and
positive/negative Infinity produce distinct, deterministic cache-key
representations instead of collapsing to "null". Use explicit type tags or
another JSON-wire-compatible canonicalization, and add regression coverage
confirming at least null and undefined remain distinct.

In `@apps/desktop/src/renderer/webclient/adapter/infra/coalescingReadCache.ts`:
- Around line 12-19: Update the cache entry lifecycle in get and the
corresponding write path to schedule eviction when each entry’s TTL reaches
expiresAt. Remove the entry only if the map still contains that exact entry, so
newer values for the same key are preserved; retain the existing read-time
expiry check as a fallback.

In `@apps/desktop/src/renderer/webclient/adapter/infra/projectState.ts`:
- Around line 50-62: Update getProjectId() so that when project is bound but
neither boundProjectId nor a matching catalogProjects entry exists, it returns
null instead of falling back to client.getStatus().activeProjectId. Preserve the
existing bound ID and catalog match precedence, and retain the active-project
fallback only when no project is bound.

In `@apps/desktop/src/renderer/webclient/adapter/misc.ts`:
- Around line 181-185: Update the sync adapter object around getStatus and
getLocalStatus so routed status and physical-local status are represented
separately. Ensure useSyncConnections can detect when ADE Web is bound to
another machine, and expose device management as unavailable in that case rather
than enabling unsupported no-op mutations.
🪄 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: 676f397e-6081-49e6-b322-836479d97307

📥 Commits

Reviewing files that changed from the base of the PR and between 9eb5fa8 and 37ae7d8.

⛔ Files ignored due to path filters (4)
  • docs/ARCHITECTURE.md is excluded by !docs/**
  • docs/features/remote-runtime/README.md is excluded by !docs/**
  • docs/features/sync-and-multi-device/README.md is excluded by !docs/**
  • docs/features/web-client/README.md is excluded by !docs/**
📒 Files selected for processing (47)
  • apps/ade-cli/src/bootstrap.ts
  • apps/ade-cli/src/cli.test.ts
  • apps/ade-cli/src/cli.ts
  • apps/ade-cli/src/multiProjectRpcServer.ts
  • apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts
  • apps/ade-cli/src/services/account/accountMachinePublisherService.ts
  • apps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.ts
  • apps/ade-cli/src/services/sync/syncCloudRelayStore.test.ts
  • apps/ade-cli/src/services/sync/syncCloudRelayStore.ts
  • apps/ade-cli/src/services/sync/syncHostService.ts
  • apps/ade-cli/src/services/sync/syncLoopbackCollision.test.ts
  • apps/ade-cli/src/services/sync/syncPeerService.test.ts
  • apps/ade-cli/src/services/sync/syncPeerService.ts
  • apps/ade-cli/src/services/sync/syncService.ts
  • apps/ade-cli/src/services/sync/syncTunnelClientService.test.ts
  • apps/ade-cli/src/services/sync/syncTunnelClientService.ts
  • apps/desktop/src/main/services/ipc/registerIpc.ts
  • apps/desktop/src/main/services/ipc/runtimeBridge.test.ts
  • apps/desktop/src/main/services/sync/syncService.test.ts
  • apps/desktop/src/preload/global.d.ts
  • apps/desktop/src/preload/preload.test.ts
  • apps/desktop/src/preload/preload.ts
  • apps/desktop/src/renderer/browserMock.ts
  • apps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsx
  • apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx
  • apps/desktop/src/renderer/components/settings/useSyncConnections.ts
  • apps/desktop/src/renderer/webclient/adapter/__tests__/adapter.test.ts
  • apps/desktop/src/renderer/webclient/adapter/__tests__/connectionsPane.test.tsx
  • apps/desktop/src/renderer/webclient/adapter/files.ts
  • apps/desktop/src/renderer/webclient/adapter/index.ts
  • apps/desktop/src/renderer/webclient/adapter/infra/cacheKey.ts
  • apps/desktop/src/renderer/webclient/adapter/infra/coalescingReadCache.ts
  • apps/desktop/src/renderer/webclient/adapter/infra/commandCaller.ts
  • apps/desktop/src/renderer/webclient/adapter/infra/invalidation.ts
  • apps/desktop/src/renderer/webclient/adapter/infra/projectState.ts
  • apps/desktop/src/renderer/webclient/adapter/lanes.ts
  • apps/desktop/src/renderer/webclient/adapter/misc.ts
  • apps/desktop/src/renderer/webclient/adapter/project.ts
  • apps/desktop/src/renderer/webclient/adapter/prs.ts
  • apps/desktop/src/renderer/webclient/adapter/remoteRuntime.ts
  • apps/desktop/src/renderer/webclient/shell/WebClientRoot.tsx
  • apps/desktop/src/renderer/webclient/sync/__tests__/sync.test.ts
  • apps/desktop/src/renderer/webclient/sync/connection.ts
  • apps/desktop/src/shared/ipc.ts
  • apps/desktop/src/shared/types/sync.ts
  • apps/ios/ADE/Services/SyncService.swift
  • apps/ios/ADETests/ADETests.swift

Comment thread apps/ade-cli/src/services/sync/syncCloudRelayStore.ts
Comment thread apps/ade-cli/src/services/sync/syncCloudRelayStore.ts Outdated
Comment thread apps/ade-cli/src/services/sync/syncCloudRelayStore.ts Outdated
Comment thread apps/ade-cli/src/services/sync/syncTunnelClientService.ts Outdated
Comment thread apps/desktop/src/renderer/webclient/adapter/__tests__/connectionsPane.test.tsx Outdated
Comment thread apps/desktop/src/renderer/webclient/adapter/infra/cacheKey.ts
Comment thread apps/desktop/src/renderer/webclient/adapter/infra/coalescingReadCache.ts Outdated
Comment thread apps/desktop/src/renderer/webclient/adapter/infra/projectState.ts
Comment thread apps/desktop/src/renderer/webclient/adapter/misc.ts
…tatus

Address review threads on PR #846:
- syncCloudRelayStore: regenerate machineKey+secret as one atomic pair;
  scope the rotation lock to a live PID/token owner (no age-only steal);
  serialize every whole-file config write through that lock.
- syncTunnelClientService: key the claim cache by (relay origin, machine
  key) so a rotated identity forces a fresh claim.
- webclient cacheKey: type-tagged, key-sorted, injective encoding that
  distinguishes missing/undefined/null/non-finite args (no collisions).
- coalescingReadCache: evict resolved entries when their TTL expires.
- projectState: a bound-but-unresolved project returns null, never
  another project's active id while switching.
- webclient misc: keep routed host status distinct from physical local
  ADE Web status; disable local device management while remote-bound.
- Tests restore window.ade / dispose the adapter in finally.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@arul28

arul28 commented Jul 17, 2026

Copy link
Copy Markdown
Owner Author

@codex review

Pushed da6c79251 addressing all 9 review threads in one batch:

  • syncCloudRelayStore: atomic machineKey+secret regeneration, PID/token-scoped rotation lock (no age-only steal), all whole-file writes serialized through the lock.
  • syncTunnelClientService: claim cache keyed by (relay origin, machine key) so a rotated identity forces a fresh claim.
  • cacheKey: injective type-tagged, key-sorted encoding (distinguishes missing/undefined/null/non-finite) — no key collisions.
  • coalescingReadCache: TTL-expiry evicts resolved entries.
  • projectState: bound-but-unresolved project returns null, never another project's active id mid-switch.
  • misc: routed host status kept distinct from physical local ADE Web status; local device mgmt disabled while remote-bound.
  • Tests restore window.ade / dispose the adapter in finally.

Note: the test-desktop (8) red is a pre-existing fake-timer flake in agentChatService.test.ts (cron scheduling, from #844) — byte-identical to main where CI is green; untouched by this PR.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: da6c792515

ℹ️ 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 thread apps/desktop/src/renderer/webclient/adapter/infra/cacheKey.ts
Comment thread apps/desktop/src/renderer/webclient/adapter/misc.ts Outdated
Comment thread apps/desktop/src/renderer/components/settings/useSyncConnections.ts Outdated
…me is down

Codex re-review of da6c792 surfaced:
- cacheKey encoder: the typeof switch had no exhaustive tail, so tsc
  (TS2366) failed — add a throwing default for unreachable typeof values.
- misc localSyncSnapshot used "idle", not a SyncTailnetDiscoveryState
  (TS2322) — use "disabled" for the web client's inactive discovery.
- useSyncConnections.refresh: settle getStatus/listDevices independently
  from getLocalStatus so a remote-bound window whose routed runtime is
  offline still renders the physical Mac instead of a load error; routed
  context is kept best-effort and never clears the local snapshot.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@arul28

arul28 commented Jul 17, 2026

Copy link
Copy Markdown
Owner Author

@codex review

Pushed ce03fbdd1 — fixes your 3 threads on da6c79251:

  • cacheKey.ts (P1 TS2366): added a throwing default so the typeof switch is exhaustive; npm run typecheck is green.
  • misc.ts (P1 TS2322): "idle""disabled" (a valid SyncTailnetDiscoveryState) for the web client's inactive discovery.
  • useSyncConnections.ts (P2): refresh() now settles getStatus/listDevices independently from getLocalStatus — a remote-bound window whose routed runtime is offline still renders the physical Mac; routed status/devices are best-effort and never clear the local snapshot or raise a load error.

Only remaining red is test-desktop (8) — the pre-existing fake-timer flake in agentChatService.test.ts (cron, from #844), byte-identical to main where CI is green.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
apps/ade-cli/src/services/sync/syncCloudRelayStore.test.ts (1)

64-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the invalid-machineKey case independently.

This only exercises an invalid secret, despite requiring both credentials to rotate when either half is invalid. Add the converse case with a valid secret and malformed machineKey.

Proposed additional coverage
+  it("regenerates both halves when the persisted machine key is invalid", () => {
+    const oldSecret = "b".repeat(48);
+    fs.writeFileSync(filePath, `${JSON.stringify({
+      machineKey: "too-short",
+      secret: oldSecret,
+      relayUrl: "https://relay.example.com",
+    })}\n`);
+
+    const repaired = createSyncCloudRelayStore({ filePath }).getConfig();
+
+    expect(repaired.machineKey).toMatch(/^[a-f0-9]{32}$/);
+    expect(repaired.secret).toMatch(/^[a-f0-9]{48}$/);
+    expect(repaired.secret).not.toBe(oldSecret);
+  });
🤖 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/syncCloudRelayStore.test.ts` around lines 64 -
78, Add an independent test alongside the existing regeneration test for a valid
48-character hexadecimal secret paired with a malformed machineKey, then call
createSyncCloudRelayStore({ filePath }).getConfig() and assert both credentials
are regenerated, including that the new machineKey differs from the persisted
invalid value and the repaired configuration is written back.
🤖 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/sync/syncCloudRelayStore.ts`:
- Around line 232-245: Update releaseRotationLock so the finally block does not
return when the current owner token differs from lease.owner.token. Instead,
conditionally perform the unlinkSync cleanup only when the tokens match,
preserving the existing ENOENT handling and allowing any closeSync exception to
propagate.

---

Nitpick comments:
In `@apps/ade-cli/src/services/sync/syncCloudRelayStore.test.ts`:
- Around line 64-78: Add an independent test alongside the existing regeneration
test for a valid 48-character hexadecimal secret paired with a malformed
machineKey, then call createSyncCloudRelayStore({ filePath }).getConfig() and
assert both credentials are regenerated, including that the new machineKey
differs from the persisted invalid value and the repaired configuration is
written back.
🪄 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: cb8a5206-7b6b-4355-ac99-5640e764221c

📥 Commits

Reviewing files that changed from the base of the PR and between 37ae7d8 and ce03fbd.

📒 Files selected for processing (11)
  • apps/ade-cli/src/services/sync/syncCloudRelayStore.test.ts
  • apps/ade-cli/src/services/sync/syncCloudRelayStore.ts
  • apps/ade-cli/src/services/sync/syncTunnelClientService.test.ts
  • apps/ade-cli/src/services/sync/syncTunnelClientService.ts
  • apps/desktop/src/renderer/components/settings/useSyncConnections.ts
  • apps/desktop/src/renderer/webclient/adapter/__tests__/adapter.test.ts
  • apps/desktop/src/renderer/webclient/adapter/__tests__/connectionsPane.test.tsx
  • apps/desktop/src/renderer/webclient/adapter/infra/cacheKey.ts
  • apps/desktop/src/renderer/webclient/adapter/infra/coalescingReadCache.ts
  • apps/desktop/src/renderer/webclient/adapter/infra/projectState.ts
  • apps/desktop/src/renderer/webclient/adapter/misc.ts
🚧 Files skipped from review as they are similar to previous changes (6)
  • apps/desktop/src/renderer/webclient/adapter/tests/connectionsPane.test.tsx
  • apps/desktop/src/renderer/webclient/adapter/infra/coalescingReadCache.ts
  • apps/desktop/src/renderer/webclient/adapter/infra/projectState.ts
  • apps/ade-cli/src/services/sync/syncTunnelClientService.test.ts
  • apps/desktop/src/renderer/components/settings/useSyncConnections.ts
  • apps/ade-cli/src/services/sync/syncTunnelClientService.ts

Comment thread apps/ade-cli/src/services/sync/syncCloudRelayStore.ts
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: ce03fbdd11

ℹ️ 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".

CodeRabbit: the finally block's early return could swallow a pending
fs.closeSync exception (noUnsafeFinally). Nest the best-effort unlink
under the ownership-token check instead of returning.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@arul28

arul28 commented Jul 17, 2026

Copy link
Copy Markdown
Owner Author

@codex review

Pushed the final review fix (syncCloudRelayStore release lock): nested the best-effort unlink under the ownership-token check so the finally block no longer returns early and can't swallow a pending fs.closeSync exception (CodeRabbit noUnsafeFinally). ade-cli typecheck green, relay-store tests 16/16.

The only remaining CI red is test-desktop (8) — the pre-existing fake-timer flake in agentChatService.test.ts (hasActiveWorkloads cron tests, from #844), byte-identical to main where CI is green and untouched by this PR.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 63da993a8c

ℹ️ 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".

// two host intervals avoids a second app-level ping/pong cycle on healthy
// relay connections while still probing well inside the host's mobile miss
// budget.
return UInt64(serverMs * 2) * 1_000_000

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep iOS silence probes inside the host miss budget

When the iOS client is on an expensive/constrained path and inbound traffic stops without a close event, this doubled fallback interval is passed into syncShouldProbeTransportAfterHeartbeatSilence, which multiplies it again (3x/4x). That delays the transport probe to 6 or 8 host heartbeat intervals, while syncHostService drops mobile peers after 6 missed host heartbeats, so constrained clients can wait until after the host has already timed them out before forcing a reconnect. Keep the probe threshold based on the advertised host interval, or otherwise cap the fallback probe below the host mobile miss limit.

Useful? React with 👍 / 👎.

@arul28
arul28 merged commit fc02658 into main Jul 17, 2026
32 of 34 checks passed
@arul28
arul28 deleted the ade/connections-relay-fixes-0f4656c2 branch July 17, 2026 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant