fix(desktop): preserve OAuth sessions in sidebar - #67578
Conversation
|
Thanks for the focused fix. The premise is present on current main: The PR replaces both affected call sites through one extracted helper and covers that helper's primary-route and fallback behavior in This is an automated hermes-sweeper review. |
0db6a32 to
6404ee0
Compare
OutThisLife
left a comment
There was a problem hiding this comment.
Rebased onto current main (import-order conflict only, 6404ee0) and kicked CI, which had never run on this branch.
Premise confirmed independently. On main both fetchProfilesSessionSlice (main.ts:9780) and mergeRemoteProfileSessions (main.ts:9800) call fetchJson(url, primary.token, …). For an OAuth-gated primary primary.token is null, the call 401s, and the .catch(() => ({ sessions: [], total: 0, profile_totals: {} })) immediately below turns that into an empty sidebar. Rows stay in the backend and stay openable by id, which is exactly the reported "my sessions disappeared" shape.
Routing through fetchJsonForProfile is the right fix rather than re-deriving transport selection at the call site — that helper already picks native-bearer vs cookie-partition, and the decision itself is pinned by resolveOauthRestAuth in native-auth-decisions.ts.
Verified locally on 2b0fb72ac, stacked with #72740: 799 electron tests pass, npm run typecheck green.
This closes the fourth of the four defects listed in the #58108 sweeper review.
On the adversarial audit above: the first two points don't hold. "Contains zero references to OAuth" is the point of the change — OAuth transport selection lives inside fetchJsonForProfile, and duplicating it at the call site is what #68169 does wrong (it drops the native-bearer branch and leaves cookieless native sessions 401ing). The third point is fair as far as it goes: the new test injects a fake and proves the extraction seam, not the transport choice. That choice is covered separately by native-auth-decisions.test.ts, so the contract is pinned — just in two files rather than one.
Credit where this started: @sealca reported #41529 and wrote the original fix in #41530 against the pre-TypeScript main.cjs. Squash body on merge should carry Co-authored-by: for @sealca and @rod-nxtlevel (#58108), Closes #41529, Supersedes #41530, #68169.
|
Superseded by #72835 — not because anything was wrong with this, but because it turned out to be one of four parts of the same subsystem fix, and landing them separately would have left three overlapping routing predicates in Your commit is cherry-picked verbatim into that PR, so authorship stays with you in git history, and #41529 closes with it. Sorry for the churn after approving and rebasing this one — the consolidation call came after. |
Summary
Fixes #41529 and supersedes the stale implementation in #41530.
Hermes Desktop's primary/global session list bypassed its existing auth-aware request helper in two places:
Both paths called the token transport directly. For an OAuth-backed primary gateway, the token is
null, so the request failed and the existing fallback silently returned an empty list. Sessions remained in the backend and could still be opened by ID, but disappeared from the sidebar.Change
fetchJsonForProfile(null, ...)path, which already selects OAuth-session or token transport from the resolved connection.Proof
vitest run --project electron: 460 passed, 1 skipped.tsc -p tsconfig.electron.json --noEmit: passed.git diff --check: passed.Runtime reproduction
On current
main, with a global OAuth backend plus named remote profile overrides:/api/profiles/sessions/sidebarreturned zero recent sessions;/api/profiles/sessions?profile=defaultpath returned the persisted sessions.The difference is the primary fetch transport selected by Electron's remote-profile interception path.