fix(desktop): route session fetches by auth mode - #41530
Conversation
|
The extraction makes the routing decision testable independently from Electron's main process. The fetchTokenJson/fetchOauthJson injection is clean. One thing I noticed: mergeRemoteProfileSessions has a .catch() that falls back to an empty result set. If the session is expired or the remote is unreachable, the user gets an empty session list with no indication anything went wrong. A warning log in the catch handler would give some visibility. But thats scope for a follow up. The routing refactor itself is solid. |
e2e47ed to
924a80f
Compare
|
Thanks — agreed. I kept this PR scoped to the auth-mode routing fix, but surfacing remote session fetch failures instead of silently returning an empty list would make sense as a follow-up. |
austinpickett
left a comment
There was a problem hiding this comment.
Reviewed by Hermes Agent — Approve ✅
Good DRY fix. mergeRemoteProfileSessions() was hand-rolling the primary backend fetch with ensureBackend(null) + fetchJson(...), which bypasses the auth-mode routing that the remote-profile path already gets. Replacing it with the existing fetchJsonForProfile(null, ...) helper means the primary/global backend now uses the correct OAuth session path when it's OAuth-authenticated — same routing the remotes already use. The .catch(() => ({ sessions: [], total: 0, profile_totals: {} })) fallback is preserved. Consistent and minimal. LGTM.
|
Thanks for the focused routing fix. The underlying defect is still present on current main, but the desktop entrypoint moved during the TypeScript migration. Problems
Suggested changes
This is an automated hermes-sweeper review. |
Related to #41529. The fix is still needed, but this branch edits retired |
|
I opened #67578 as a current-main replacement for this fix. It ports the OAuth-aware primary fetch to apps/desktop/electron/main.ts, also covers the batched sidebar call site added by #67245, and adds focused regression tests. I preserved the original issue linkage and credited this PR as the superseded implementation. |
|
Superseded by #67578. You had this right first — Your authorship is credited in the squash body, and #41529 closes with it. Thanks for the original diagnosis and fix. |
What does this PR do?
Fixes Desktop's merged remote-profile session list so the primary/global backend uses the correct REST auth path when it is OAuth-authenticated.
When at least one named remote profile is configured, Desktop intercepts
GET /api/profiles/sessionsand merges rows from the primary backend plus each remote profile. Currentmainalready hasfetchJsonForProfile(), which routes requests byconn.authModeand uses the OAuth session path when needed. The primary/backend fetch insidemergeRemoteProfileSessions()still bypassed that routing and always called the token-basedfetchJson()helper.For OAuth remotes,
primary.tokencan benull, the OAuth session cookie is not attached, the request fails, and the merge fallback silently treats the primary backend as empty. The result is a sidebar that omits the global/default backend sessions even though they still exist on the remote backend.This PR keeps the change minimal: the merge-path primary fetch now goes through
fetchJsonForProfile(null, ...)instead of manually calling the token-only helper.Related Issue
Fixes #41529
Type of Change
Changes Made
mergeRemoteProfileSessions()so the primary/global backend fetch respects the resolved backend auth mode viafetchJsonForProfile(null, ...).mainalready has the right request-routing helper.How to Test
Verified locally from a clean worktree based on current upstream
main(c3055d618):cd apps/desktop node --check electron/main.cjs npm run test:desktop:platformsResults:
node --check electron/main.cjs: passed.npm run test:desktop:platforms: 102 passed, 0 failed.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
N/A. The fix is in Electron main-process request routing. Test output is summarized above.