feat(desktop): Connections panel + Account page + provider-aware avatar#819
Conversation
Post-login desktop UX for the accounts initiative. New renderer preload bridge for the account daemon actions (status/startLogin/pollLogin/signOut only; getToken not exposed). Provider-aware sidebar avatar routing to a new /account page (rich signed-out card + signed-in identity/machines/sessions + GitHub repo bridge). The three top-bar chips collapse into one Connections panel (Machines/Mobile/Web tabs); Machines extends RemoteTargetList with account machines merged. Graceful degrade when the directory Worker is not deployed. Verified by driving the renderer; screenshots in the proof drawer; typecheck + lint green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 19 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 token-free ADE account authentication and machine-directory access, introduces account status and login UI, routes account pages separately from projects, and consolidates remote, mobile, web, and account machines into a unified Connections panel. ChangesADE account contracts and bridge
Renderer account experience
Unified Connections panel
Account machines in remote targets
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 |
…s to Connections panel + /account avatar The Connections-panel refactor consolidated the per-service status chips into a single "Connections and usage" trigger + ConnectionsPanel (machines/mobile/web tabs) and turned the sidebar avatar into a /account link. Update the 16 stale renderer tests to the new accessible names/flow; no production code changed, coverage preserved (no skip/only, no test deletion). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@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: 0accd270d1
ℹ️ 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".
| name: machine.name ?? hostInfo.host, | ||
| hostname: hostInfo.host.replace(/\.$/, ""), | ||
| sshUser: null, | ||
| port: hostInfo.port, |
There was a problem hiding this comment.
For account-directory LAN/tailnet machines, this endpoint port is the ADE sync/runtime endpoint, not an SSH port (the directory tests register LAN endpoints on 8787, and the existing discovery path converts those service endpoints separately while SSH routes use a null/default SSH port). Passing it into RemoteRuntimeTargetInput.port saves an SSH target on that service port, so clicking Connect for a normal account machine attempts SSH to 8787 and fails. Create a paired target from the account identity or leave the SSH port unset unless the directory supplies explicit SSH metadata.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…P1) + fix account-machine SSH port (P2) P1 (Greptile, security): the machine account bearer in accountBridge.listMachines was sent to a baseUrl that a per-project ACCOUNT_DIRECTORY_URL secret could point at any host. Add parseTrustedDirectoryBaseUrl (https-only, http allowed on loopback) and resolve machine-level env FIRST, per-project secret only as fallback — a project can no longer redirect the machine token, and it is only ever attached to a trusted https/loopback origin. Adds a focused unit test. P2 (Codex, correctness): connectAccountMachine passed the account machine's ADE service endpoint port (e.g. 8787) as the SSH port, so Connect SSHed to the service port and failed. Leave the SSH port null (default 22), matching discoveredTargetInput; adds a regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e0c2db5ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const raw = | ||
| process.env.ADE_ACCOUNT_DIRECTORY_URL?.trim() | ||
| || readProjectSecret(projectRoot, DIRECTORY_URL_SECRET) | ||
| || null; | ||
| return parseTrustedDirectoryBaseUrl(raw); |
There was a problem hiding this comment.
When ADE_ACCOUNT_DIRECTORY_URL is unset, this falls back to the active project's ACCOUNT_DIRECTORY_URL, and parseTrustedDirectoryBaseUrl accepts any HTTPS host. Because listMachines() later attaches Authorization: Bearer ${token} to ${baseUrl}/account/machines, opening the Connections panel in a project that contains or sets that secret to https://attacker.example sends the machine account token to that host. Keep this endpoint fixed to ADE-controlled infrastructure, or allow-list the origin before attaching the bearer.
Useful? React with 👍 / 👎.
| for (const endpoint of machine.reachableEndpoints) { | ||
| const host = endpoint.host ?? endpoint.url ?? null; | ||
| const identity = routeIdentity(host, endpoint.port ?? null); | ||
| if (identity) identities.add(identity); |
There was a problem hiding this comment.
When account-directory endpoints advertise the ADE service port (the directory registration path uses LAN endpoints like host: mac.local, port: 8787), including that port in the route identity prevents matching an existing saved SSH target for the same host, whose identity is host:22/default. In that scenario the Connections panel shows a duplicate account row instead of reusing the saved target and its doctor state. Treat account endpoint ports as non-SSH for matching, or normalize them against saved target ports unless the endpoint explicitly represents SSH.
Useful? React with 👍 / 👎.
Codex re-review of the prior fix surfaced the next instance of each class: P1 (accountBridge): env-first still fell back to the per-project ACCOUNT_DIRECTORY_URL secret when ADE_ACCOUNT_DIRECTORY_URL was unset, so a project could still point the directory at an https host it controls. Resolve the machine account bearer's destination from the machine-level env ONLY; the project secret is no longer consulted, so an opened project can never redirect the token. P2 (remoteMachineModel): accountMachineRouteIdentities keyed dedup on the advertised service port (e.g. 8787), so an account machine never matched its saved SSH target (:22) and showed a duplicate row. Key identities on host at the default SSH port, ignoring the service port. Adds a regression test with a realistic 8787 endpoint. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5f2fb4c67
ℹ️ 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".
| // Machine-level routes (personal chats, account) are not project surfaces; | ||
| // the route-restore below would otherwise clobber them with the active | ||
| // project's stored route on load. | ||
| if (isPersonalChatsRoute || isAccountRoute) return; |
There was a problem hiding this comment.
When /account is open, this effect now skips project-route restoration, but TopBar only calls leavePersonalChatsRoute() before switching tabs. In that state, clicking another local or remote project changes the active binding while the URL remains /account, so the user still sees the Account page instead of the selected project until they manually navigate away.
Useful? React with 👍 / 👎.
| if (machine.online) { | ||
| sections.available.push(row); | ||
| } else { | ||
| sections.unavailable.push(row); |
There was a problem hiding this comment.
When an online account machine only advertises relay endpoints, this puts it in AVAILABLE even though AccountMachineRow deliberately refuses relay endpoints for Connect (accountMachineConnectHost skips them). Those rows are shown as available/reachable but have no Connect button and no offline explanation, so relay-only account machines become dead rows in the Connections panel.
Useful? React with 👍 / 👎.
…lay-only account machines out of AVAILABLE (P2) P1 (Codex, account routing): switching project tabs while /account was open changed the active binding but left the URL on /account, so the user kept seeing the Account page. Derive isAccountRoute in AppShell and plumb accountRouteActive into TopBar; generalize leavePersonalChatsRoute → leaveMachineRoute so a project-tab switch also leaves the /account machine route (same stored-route replay as /chats). P2 (Codex, relay-only bucketing): an online account machine advertising only relay endpoints has no directly-connectable host (accountMachineConnectHost skips relay), so it appeared in AVAILABLE as a dead row with no Connect and no explanation. Add accountMachineHasDirectRoute; keep relay-only online machines out of AVAILABLE, and have AccountMachineRow show "relay only" copy instead of a false offline/asleep label. Adds bucketing regression tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a99a203b4c
ℹ️ 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".
| }); | ||
| for (const endpoint of ranked) { | ||
| if (endpoint.kind === "relay") continue; | ||
| const host = endpoint.host ?? endpoint.url ?? null; |
There was a problem hiding this comment.
When the directory returns a direct lan/tailnet endpoint with only url populated (the shared type and mapMachine both allow this), this falls back to the whole URL string as the SSH host. For values like https://100.92.14.3:8787, connectAccountMachine then saves that full URL as RemoteRuntimeTargetInput.hostname, so SSH and saved-target dedupe operate on an invalid hostname instead of 100.92.14.3. Parse endpoint.url to a hostname for non-relay endpoints, and apply the same normalization in the account-machine matching helper.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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/TopBar.tsx (1)
1152-1164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
!showSyncControlnow closes the entire unified Connections panel, including the Machines tab.Previously, sync/web drawer state was separate from the Machines drawer; now
setConnectionsOpen(false)fires unconditionally whenevershowSyncControl(projectHydrated === true) goes false, closing the panel regardless of which tab is open. Since SSH machine management is account/user-scoped, not project-scoped, a project-hydration transition (e.g., mid project switch) shouldn't force-close an open Machines tab where a user might be mid add/remove-machine flow.♻️ Proposed fix
+ const connectionsTabRef = useRef(connectionsTab); + connectionsTabRef.current = connectionsTab; + useEffect(() => { ... if (!showSyncControl) { setSyncSnapshot(null); - setConnectionsOpen(false); + if (connectionsTabRef.current !== "machines") setConnectionsOpen(false); return () => { cancelled = true; }; }Consider also adding a test asserting the Machines tab stays open across a
projectHydratedflip (none of the providedTopBar.test.tsxcases exercise this path).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/app/TopBar.tsx` around lines 1152 - 1164, Update the !showSyncControl branch in the TopBar useEffect so it clears sync-specific state without unconditionally calling setConnectionsOpen(false), preserving an open Machines tab during project-hydration changes. Ensure the sync/web panel still closes when appropriate through tab-specific logic, and add coverage for keeping the Machines tab open across a projectHydrated flip if the existing test structure supports it.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/lib/accountLogin.ts (1)
16-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAd-hoc
window.ade.accountcasts duplicated across three sites instead of one shared accessor.Each site redeclares its own narrow
(window.ade as typeof window.ade & { account?: {...} }).accountcast covering only the methods it needs.lib/account.tsalready has an internalaccountApi()-style helper covering the full contract; exporting and reusing a single typed accessor would remove this duplication and reduce the risk of the ad-hoc shapes drifting from the real preload contract over time.
apps/desktop/src/renderer/lib/accountLogin.ts#L16-L28: replace the localaccountApi()(startLogin/pollLogin/cancelLogin subset) with a shared export fromlib/account.ts.apps/desktop/src/renderer/components/account/AccountPage.tsx#L266-L283: replaceMachinesGlance's inlinelistMachinescast with the same shared accessor.apps/desktop/src/renderer/components/account/AccountPage.tsx#L487-L500: replacehandleSignOut's inlinesignOutcast with the same shared accessor.🤖 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/lib/accountLogin.ts` around lines 16 - 28, Export the existing full-contract account API accessor from lib/account.ts and reuse it everywhere instead of maintaining ad-hoc window.ade.account casts. In apps/desktop/src/renderer/lib/accountLogin.ts lines 16-28, replace the local accountApi covering startLogin, pollLogin, and cancelLogin with the shared accessor; in apps/desktop/src/renderer/components/account/AccountPage.tsx lines 266-283 and 487-500, replace the inline listMachines and signOut casts with that same accessor.apps/desktop/src/renderer/components/account/AccountPage.tsx (1)
487-490: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAd-hoc
window.ade.accountcast duplicated a third time.Same narrowed cast pattern as above — see consolidated comment.
🤖 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/account/AccountPage.tsx` around lines 487 - 490, Update handleSignOut to remove the duplicated ad-hoc window.ade.account type cast and reuse the existing shared account API typing or accessor established by the consolidated change. Preserve the current signOut behavior while centralizing this narrowing instead of declaring another inline account type.
🤖 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/desktop/src/main/services/account/accountBridge.ts`:
- Around line 75-93: Update parseTrustedDirectoryBaseUrl to reject URLs
containing username/password credentials, search queries, or hash fragments,
while preserving the existing HTTPS and loopback-HTTP protocol checks. Normalize
the accepted value from the parsed URL rather than the raw trimmed input,
removing trailing slashes from the parsed pathname/base URL so appending
endpoint paths always targets the intended directory path.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 8489-8517: Update the account IPC handlers registered in the
accountStatus/accountStartLogin/accountPollLogin/accountCancelLogin/accountSignOut/accountListMachines
block to use channel-specific trace redaction for both arguments and results,
preventing OAuth URLs, session IDs, and account PII from reaching the generic
IPC tracer. Reuse the existing IPC tracing/redaction mechanism and apply it only
to these account channels while preserving their current bridge calls and return
types.
In `@apps/desktop/src/renderer/browserMock.ts`:
- Around line 3219-3228: Update the account mock methods around pollLogin,
status, and signOut to persist authentication transitions in the underlying mock
storage: successful pollLogin must store the signed-in state, while signOut must
store the signed-out state. Ensure subsequent status checks and refreshes
reflect the latest transition instead of the initial ade.mock.account value.
In `@apps/desktop/src/renderer/components/account/AccountPage.tsx`:
- Around line 487-500: Update handleSignOut to catch failures from
api.signOut(), preserve the signing-out reset in finally, and provide the user
with an appropriate error indication while keeping the existing successful
status update flow unchanged.
In `@apps/desktop/src/renderer/components/app/ConnectionsPanel.tsx`:
- Around line 180-184: Update the ConnectionsPanel effect around
loadAccountMachines so account machines are refreshed whenever the Machines tab
becomes active, not only when the component mounts. Tie the refresh to the
existing active-tab state or tab-change handler, while preserving the initial
load and avoiding refreshes for Mobile or Web tabs.
In `@apps/desktop/src/renderer/lib/account.ts`:
- Around line 31-34: Update fetchAccountStatus to maintain a monotonically
increasing request serial for each initiated fetch, including forced requests.
Capture the serial per request and make its completion logic clear inFlight and
call emit only when that serial is still the latest, preventing older requests
from overwriting newer tracking or publishing stale statuses.
In `@apps/desktop/src/renderer/lib/accountLogin.ts`:
- Around line 72-87: Update the login flow around api.startLogin() to re-check
cancelledRef.current immediately after the promise resolves and before assigning
sessionIdRef.current or calling openExternalUrl; return without side effects
when cancellation occurred. Also ensure the subsequent setPhase("awaiting") path
is skipped for the cancelled flow, while preserving normal session setup and
polling for active logins.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/app/TopBar.tsx`:
- Around line 1152-1164: Update the !showSyncControl branch in the TopBar
useEffect so it clears sync-specific state without unconditionally calling
setConnectionsOpen(false), preserving an open Machines tab during
project-hydration changes. Ensure the sync/web panel still closes when
appropriate through tab-specific logic, and add coverage for keeping the
Machines tab open across a projectHydrated flip if the existing test structure
supports it.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/account/AccountPage.tsx`:
- Around line 487-490: Update handleSignOut to remove the duplicated ad-hoc
window.ade.account type cast and reuse the existing shared account API typing or
accessor established by the consolidated change. Preserve the current signOut
behavior while centralizing this narrowing instead of declaring another inline
account type.
In `@apps/desktop/src/renderer/lib/accountLogin.ts`:
- Around line 16-28: Export the existing full-contract account API accessor from
lib/account.ts and reuse it everywhere instead of maintaining ad-hoc
window.ade.account casts. In apps/desktop/src/renderer/lib/accountLogin.ts lines
16-28, replace the local accountApi covering startLogin, pollLogin, and
cancelLogin with the shared accessor; in
apps/desktop/src/renderer/components/account/AccountPage.tsx lines 266-283 and
487-500, replace the inline listMachines and signOut casts with that same
accessor.
🪄 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: 3da1aa5c-054a-4c4c-8bac-00d42298975a
📒 Files selected for processing (25)
apps/desktop/src/main/services/account/accountBridge.trust.test.tsapps/desktop/src/main/services/account/accountBridge.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/account/AccountPage.tsxapps/desktop/src/renderer/components/app/App.tsxapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/app/ConnectionsPanel.tsxapps/desktop/src/renderer/components/app/TabNav.test.tsxapps/desktop/src/renderer/components/app/TabNav.tsxapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/remoteTargets/AccountMachineRow.tsxapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsxapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsxapps/desktop/src/renderer/components/remoteTargets/remoteMachineModel.account.test.tsapps/desktop/src/renderer/components/remoteTargets/remoteMachineModel.tsapps/desktop/src/renderer/lib/account.tsapps/desktop/src/renderer/lib/accountLogin.tsapps/desktop/src/renderer/lib/connectionsPanel.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/account.tsapps/desktop/src/shared/types/index.ts
…P2 + Greptile P1) A direct lan/tailnet account endpoint may populate only `url` (e.g. http://10.0.0.9:8787), not `host`. The old `endpoint.host ?? endpoint.url` then fed the whole URL string into the SSH hostname, so Connect saved an unresolvable `RemoteRuntimeTargetInput.hostname` and dedupe keyed on the URL. Add accountEndpointHost() (parses url-valued endpoints to the hostname alone) and use it in the connect path (accountMachineConnectHost), the dedupe identities, and the has-direct-route check. Adds regression tests: URL-valued connect saves the bare host, and a URL-valued endpoint dedupes against its saved target. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ffe1720ce
ℹ️ 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".
| // leave it null so the SSH transport falls back to its default (22). | ||
| port: null, | ||
| sshKeyPath: null, | ||
| routes: null, |
There was a problem hiding this comment.
When an account machine advertises multiple direct endpoints (for example tailnet plus LAN), this saves only the single chosen hostname and drops the rest. If the preferred endpoint is stale or unreachable while another advertised route works, the remote runtime has no target.routes fallback candidates to try, unlike discovered machines that preserve all SSH routes; carry the other non-relay endpoints as routes with port: null instead of setting routes to null.
Useful? React with 👍 / 👎.
…findings CodeRabbit: - accountBridge parseTrustedDirectoryBaseUrl: reject credentials/query/fragment, return normalized origin+path (P: Minor). - registerIpc: redact account IPC payloads (session IDs, OAuth authorizeUrl, and account/machine PII) from trace logs (P: Major, security). - browserMock: persist login/sign-out transitions via the ade.mock.account flag (Minor). - AccountPage.handleSignOut: surface sign-out failures via inline error state instead of swallowing them (Minor). - ConnectionsPanel: reload account machines whenever the Machines tab opens, not just on mount (Minor). - account.ts fetchAccountStatus: request-serial guard so only the latest fetch emits/clears inFlight (Minor, race). - accountLogin: check cancelledRef after startLogin() resolves and cancel the orphaned session so unmount/cancel no longer opens an OAuth window or polls (P: Major, race). Codex: - RemoteTargetList.connectAccountMachine: carry all non-relay endpoints as target.routes (accountMachineSshRoutes) so a saved account machine keeps SSH fallback candidates like a discovered machine, instead of dropping them (P2). Tests: accountBridge trust cases (credentials/query/fragment/path), accountMachineSshRoutes ordering + relay exclusion. Whole-desktop tsc clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2eb435f36a
ℹ️ 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".
| setError("Account sign-in isn't available on this build."); | ||
| return; | ||
| } | ||
| cancelledRef.current = false; |
There was a problem hiding this comment.
When a user cancels sign-in and immediately starts another attempt before the old polling loop wakes up, this shared cancelledRef is reset to false for the new attempt, so the old loop continues polling its canceled session. That stale loop can then receive an error, clear sessionIdRef, publish the signed-out status, and set the hook phase to error while the new browser sign-in is still in progress. Use a per-attempt id/AbortController instead of one boolean shared by all login attempts.
Useful? React with 👍 / 👎.
… + Codex P2) Greptile P1 (account.ts): publishAccountStatus emitted the freshly signed-in/out status but did not advance fetchSerial, so a slow api.status() request started before the publish still matched serial===fetchSerial and re-emitted stale signed-out state over it. publishAccountStatus now bumps fetchSerial and drops the in-flight promise so any superseded fetch can no longer overwrite the published status. Adds a deterministic race regression test (account.test.ts). Codex P2 (accountLogin.ts): the cancellation used one shared cancelledRef boolean, so cancel-then-retry reset it to false and the old polling loop kept running its canceled session (eventually clearing sessionIdRef / publishing signed-out / setting phase=error under the new attempt). Replace it with a per-attempt id (attemptRef): beginLogin claims ++attemptRef, every guard checks attemptRef.current !== attemptId, and cancel/unmount/newer-attempt all bump the ref so a stale loop bails without touching shared UI state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Desktop Connections panel + Account page + provider-aware avatar
The post-login desktop UX. Consumes the merged
accountdaemon action domain (#815) via a new renderer preload bridge (window.ade.account.*—status/startLogin/pollLogin/signOutonly;getTokenis deliberately NOT exposed to the renderer; the daemon's CTO-only gate still applies).TabNav.tsx): shows the sign-in provider's image → GitHub-creds fallback → monogram; provider-tinted ring; click →/account(was: external GitHub profile)./account): signed-out = rich sign-in card (GitHub one-click + Email/Apple/Google + "local pairing needs no login"); signed-in = identity header, machines-at-a-glance (online/offline), active-sessions seam + sign-out; GitHub repo-bridge banner after GitHub sign-in./account. Machines tab extendsRemoteTargetList's CONNECTED / AVAILABLE / OFFLINE with account machines merged in (an "account" badge + a "Why offline?" doctor affordance on offline account rows).Verified by driving the real renderer — screenshots of the signed-in Account page, the Connections panel (all three sections + account badges), the signed-out card, and the provider-aware avatar are in the proof drawer. Desktop typecheck + lint green.
🤖 Generated with Claude Code
Summary by CodeRabbit
Greptile Summary
This PR lands the post-login desktop UX: a provider-aware sidebar avatar (→
/account), a full Account page (signed-out sign-in card + signed-in identity/machines/sessions), and a consolidated Connections panel (Machines / Mobile / Web tabs) that replaces three separate header chips. The bearer token is deliberately kept in the main process — only token-free status surfaces cross IPC — and the previous security concerns (bearer exfiltration, endpoint URL normalization, stale-status overwrite, account-route navigation trap) are all addressed.accountBridge.tsrestricts directory fetches toADE_ACCOUNT_DIRECTORY_URL(never per-project secrets) and validates it throughparseTrustedDirectoryBaseUrl, closing the token-exfiltration path flagged in the prior review.remoteMachineModel.ts/AccountMachineRow.tsxparse endpoint URLs to hostname-only and explicitly drop the ADE service port (8787) when building SSH targets, fixing the previous "normalize endpoint URLs" and "service port as SSH port" bugs.App.tsx/AppShell.tsx/TopBar.tsxaddisAccountRouteguards that prevent the route-restore effect from clobbering/accountand ensure project-tab clicks navigate away from the account page — resolving the prior "account route traps switches" finding.Confidence Score: 4/5
Safe to merge with one fix: the signed-in identity card in AccountPage renders a broken-image icon when the GitHub avatar URL fails instead of falling back to the monogram initials, unlike TabNav and ConnectionsPanel which both guard this correctly.
The account-route navigation, bearer-token boundary, endpoint normalization, and stale-status race are all correctly addressed. One regression stands out: the AccountPage identity card's avatar
<img>has noonErrorhandler, so a failed GitHub image load shows a broken-image icon rather than the initials fallback that every other avatar surface provides. The fix is a one-liner pattern already used twice in this same PR.apps/desktop/src/renderer/components/account/AccountPage.tsx — the identity card avatar image needs an onError handler and imgBroken state matching the pattern in TabNav.tsx and ConnectionsPanel.tsx.
Important Files Changed
<img>is missing an onError handler — broken-image icon shows instead of initials when the GitHub fallback URL fails. All other sign-in/sign-out flows look correct.Comments Outside Diff (1)
apps/desktop/src/renderer/components/app/TopBar.tsx, line 1308-1317 (link)ProjectTabHostskips/accountduring route restore, but project-tab clicks only leave/chats. When the Account page is open, clicking another local or remote project updates the active binding while the URL stays/account, so the selected project never appears.Artifacts
Repro: focused TopBar account-route trap test harness
Repro: verbose failing Vitest output showing no navigation after project tab click
Prompt To Fix With AI
Reviews (8): Last reviewed commit: "Merge branch 'main' into ade/accounts-de..." | Re-trigger Greptile