feat: add web and TUI account machine parity#825
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
arul28
changed the title
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds secure account-machine discovery, OAuth-backed browser sessions, DPoP-bound pairing, paired remote runtime support, CLI machine commands, account-aware machine-selection UI, and strict directory CORS validation. ChangesAccount machine workflow
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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
apps/desktop/src/renderer/webclient/shell/WebShell.tsx (1)
131-161: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNo busy/disabled state for in-flight account-machine pairing.
Clicking an account-machine row (
onSelect(machine)) does not disable the row or show a spinner while the async pairing/connect completes; a user could double-click and trigger duplicate pairing attempts. This mirrors the same gap for the existing environment rows above (no connecting indicator there either), so it's a pre-existing pattern rather than a new regression — flagging for awareness rather than as a blocker. Whether the ultimateonSwitchAccountMachinehandler (likelyconnectToAccountMachineinWebClientRoot.tsx, which does trackconnectingAccountMachineKey) surfaces this state elsewhere isn't visible in this diff.🤖 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/webclient/shell/WebShell.tsx` around lines 131 - 161, The account-machine rows rendered by the machines map lack an in-flight pairing state. Reuse the existing connectingAccountMachineKey state from the account-machine connection flow to identify the selected machine, disable that row while pairing is active, and display the established connecting indicator; ensure onSelect cannot trigger duplicate attempts during that period.
🤖 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/syncPairingStore.ts`:
- Around line 130-140: Update pairPeerViaAccount so accountVerified cannot grant
runtimeHostGranted based solely on the client-declared peer.deviceType. Require
an independent authenticated desktop-capability proof alongside the consumed
runtime host grant or verified same-owner attestation, while preserving mobile
allowlist behavior and one-time grant consumption.
In `@apps/ade-cli/src/tuiClient/__tests__/remoteBridge.test.ts`:
- Around line 49-50: Update the remote bridge test around the socket data
listener to accumulate Buffer chunks across multiple "data" events until the
complete newline-delimited JSON-RPC frame is received, then compare the
assembled UTF-8 payload with the expected response. Keep the existing response
assertion unchanged once the full frame has been collected.
In `@apps/ade-cli/src/tuiClient/remoteBridge.ts`:
- Around line 483-485: Update the bridge socket data handling around the
socket.on("data") callback to preserve UTF-8 characters split across TCP chunks.
Configure the socket encoding before forwarding data, or use a persistent
StringDecoder, and pass the decoded complete text to opened.write while
preserving the existing teardown behavior.
In `@apps/ade-cli/src/tuiClient/remoteLauncher.ts`:
- Around line 613-636: The try block in the remote-launch flow conflates
successful connection metadata writes with runtime initialization failures.
Update the code around initializeRemoteRpc, pairedStore.markEndpointSucceeded,
and registry.update so only connection/initialization errors enter the
compatibility handling; perform both metadata writes afterward as best-effort
operations that cannot close or invalidate the successfully initialized client.
In `@apps/desktop/src/main/services/account/accountBridge.trust.test.ts`:
- Line 146: Remove the duplicate calls declaration in the test scope, retaining
a single const calls variable with its existing type and initialization so the
TypeScript file compiles.
In `@apps/desktop/src/renderer/webclient/account/client.ts`:
- Around line 289-336: Update getAccessToken to deduplicate concurrent refreshes
using a shared in-flight refresh promise, so callers reuse one postToken request
and cannot overwrite a newer session with stale refresh results. Ensure the
promise is cleared after completion and that only confirmed invalid or
expired-session responses call expireSession; propagate transient
postToken/network failures without forcing logout.
In `@apps/desktop/src/renderer/webclient/sync/client.ts`:
- Around line 305-310: Update pairWithAccount to create a DPoP proof factory
rather than signing once before relay attempts. Move signDpopProof into the
per-candidate attempt flow so each endpoint receives a newly generated proof
with the current nonce, while preserving the existing key, device, and
access-token inputs.
In `@apps/desktop/src/renderer/webclient/sync/connection.ts`:
- Around line 228-243: Update pairWithAccount to persist the newly paired
account endpoints in this.endpoints before reconnection can be enabled, ensuring
finishConnected uses the current machine’s endpoints rather than an empty or
stale list. Apply the same endpoint assignment in the related flow around
finishConnected.
In `@apps/desktop/src/shared/accountDirectory.ts`:
- Around line 199-200: Update the account-directory response handling around
parseAccountMachinesPayload to read the response body as a stream with a strict
maximum byte limit before JSON parsing. Reject or return the existing failure
state when the limit is exceeded, then parse only the bounded body and preserve
the current successful machine parsing behavior.
- Around line 360-369: Update the stable-ID matching flow around byStableId and
stableMatch to throw an ambiguity error when byStableId contains multiple
entries, before executing the byName fallback. Preserve the existing unique
stable-ID return path and only perform name matching when no stable-ID matches
exist.
---
Nitpick comments:
In `@apps/desktop/src/renderer/webclient/shell/WebShell.tsx`:
- Around line 131-161: The account-machine rows rendered by the machines map
lack an in-flight pairing state. Reuse the existing connectingAccountMachineKey
state from the account-machine connection flow to identify the selected machine,
disable that row while pairing is active, and display the established connecting
indicator; ensure onSelect cannot trigger duplicate attempts during that period.
🪄 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: 9d68e02b-8ccc-4003-a466-2897458449ce
⛔ Files ignored due to path filters (3)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/ade-code/README.mdis excluded by!docs/**docs/features/web-client/README.mdis excluded by!docs/**
📒 Files selected for processing (39)
apps/account-directory/README.mdapps/account-directory/src/directory.tsapps/account-directory/test/directory.test.tsapps/account-directory/wrangler.jsoncapps/ade-cli/README.mdapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/ade-cli/src/services/account/accountAttestationVerifier.test.tsapps/ade-cli/src/services/account/accountMachineDirectoryService.test.tsapps/ade-cli/src/services/account/accountMachineDirectoryService.tsapps/ade-cli/src/services/sync/syncCloudRelayStore.tsapps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncPairingStore.tsapps/ade-cli/src/tuiClient/__tests__/HeaderFooter.test.tsxapps/ade-cli/src/tuiClient/__tests__/remoteBridge.test.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/components/Header.tsxapps/ade-cli/src/tuiClient/connection.tsapps/ade-cli/src/tuiClient/remoteBridge.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/adeActions/registry.tsapps/desktop/src/main/services/remoteRuntime/syncPairedMachineStore.test.tsapps/desktop/src/main/services/remoteRuntime/syncPairedMachineStore.tsapps/desktop/src/renderer/webclient/account/client.test.tsapps/desktop/src/renderer/webclient/account/client.tsapps/desktop/src/renderer/webclient/shell/MachinePicker.tsxapps/desktop/src/renderer/webclient/shell/WebClientRoot.tsxapps/desktop/src/renderer/webclient/shell/WebShell.tsxapps/desktop/src/renderer/webclient/shell/__tests__/MachinePicker.test.tsxapps/desktop/src/renderer/webclient/sync/__tests__/sync.test.tsapps/desktop/src/renderer/webclient/sync/client.tsapps/desktop/src/renderer/webclient/sync/connection.tsapps/desktop/src/shared/accountDirectory.tsapps/desktop/src/shared/types/account.tsapps/desktop/src/shared/types/sync.ts
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@codex review |
|
Addressed the CodeRabbit in-flight pairing note in cd6195b: account-machine connects now have a synchronous duplicate guard, and the machine switcher disables and labels the active row as connecting. |
![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: cd6195bb92
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4037662cbe
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fd2dea3c8
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71cbe5245c
ℹ️ 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".
| state: "auth_expired", | ||
| machines: [], | ||
| message: `Machine directory returned ${response.status}.`, | ||
| message: "Your ADE account session expired. Sign in again.", |
There was a problem hiding this comment.
This new auth_expired result is returned specifically so the UI can offer re-authentication, but the existing desktop account MachinesGlance only handles ok, not_configured, and signed_out; it falls through to the generic "Can't reach the machine directory" path for this state. When a stored account session expires while the account page still considers the user signed in, users get outage guidance instead of the intended "sign in again" path, so the new state needs to be handled in the desktop consumers as part of this change.
Useful? React with 👍 / 👎.
1 participant
Summary
ade machines listplus stable-idconnect/hopflows and surface the shared daemon account session inade codeVerification
git diff --checkpassed; no iOS diffSetup
Deployment still requires the existing Clerk/account-directory configuration, an applied D1 migration,
WEB_CLIENT_ORIGINset to the exact hosted HTTPS origin, and trustedADE_ACCOUNT_DIRECTORY_URL/ web-client build settings.Summary by CodeRabbit
Greptile Summary
This PR adds account-backed machine discovery and connection flows across the hosted web client, CLI/TUI, Worker, and paired runtime. The main changes are:
ade machines list,connect, andhopflows that pair account machines before launching ADE Code.Confidence Score: 5/5
This PR appears safe to merge from the reviewed paths.
No issues were identified in the changed account-directory CORS handling, shared endpoint validation, CLI machine commands, account pairing credential adoption, or hosted web integration paths.
No files require special attention.
What T-Rex did
Important Files Changed
ade machinesplanning, execution, and account-machine output formatting; no issues found.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Web as Hosted web / CLI participant Dir as Account directory Worker participant Relay as Verified WSS relay participant Host as ADE machine host participant Store as Paired credential store Web->>Dir: GET /account/machines (Bearer account token, exact allowed origin for browser) Dir-->>Web: Validated machine records + relay endpoints Web->>Relay: Account hello with DPoP over directory-verified WSS Relay->>Host: Relay account-authenticated hello Host->>Host: Verify Clerk owner and DPoP / existing pin Host-->>Web: hello_ok + accountPairing on first adoption Web->>Store: Persist normal paired DPoP credentials and endpoints Web->>Host: Reconnect through paired runtime bridge%%{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 Hosted web / CLI participant Dir as Account directory Worker participant Relay as Verified WSS relay participant Host as ADE machine host participant Store as Paired credential store Web->>Dir: GET /account/machines (Bearer account token, exact allowed origin for browser) Dir-->>Web: Validated machine records + relay endpoints Web->>Relay: Account hello with DPoP over directory-verified WSS Relay->>Host: Relay account-authenticated hello Host->>Host: Verify Clerk owner and DPoP / existing pin Host-->>Web: hello_ok + accountPairing on first adoption Web->>Store: Persist normal paired DPoP credentials and endpoints Web->>Host: Reconnect through paired runtime bridgeReviews (3): Last reviewed commit: "ship: refresh account machine credential..." | Re-trigger Greptile