fix(desktop): scope messaging sessions by profile - #44157
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review: PR #44157
Verdict: Approved — correct profile scoping fix for messaging sessions.
Summary
- Files changed: apps/desktop/src/app/desktop-controller.tsx (+30, -10), apps/desktop/src/app/session/hooks/use-session-actions.ts (+6, -1)
- Scopes messaging sidebar fetch and load-more to the active profile instead of always using 'all'
- Includes profile, source, and last_active in section signature for proper stale-detection
- Resolves session owner from messagingSessions and cronSessions as fallbacks
Assessment
Correctness: The fallback chain sessions -> messagingSessions -> cronSessions ensures session owner is found regardless of which list the session lives in.
No issues found.
Reviewed by Hermes Agent
68561fa to
fb1e5f9
Compare
Cherry-pick of upstream PR NousResearch#44157 (fix profile routing for messaging sessions in the Desktop sidebar). Resolve session owners from messagingSessions and cronSessions before resuming stored sessions. Fork-Origin: gitzwz Cherry-Picked-From: NousResearch#44157 Co-Authored-By: Codex <noreply@openai.com>
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing both the sidebar fetch and resume-owner paths. The underlying bug still exists on current main, but this branch needs a targeted port before it is safe to salvage.
Problems
- The production fetches were extracted by
25c7900fbintoapps/desktop/src/app/session/hooks/use-session-list-actions.ts; currentrefreshMessagingSessionsandloadMoreMessagingForPlatformstill hardcodeprofile="all"at lines 102 and 126, whilerefreshCronSessionshas the same sibling defect at line 86. - The current regular-recents path guards stale async responses with
refreshSessionsRequestRefatuse-session-list-actions.ts:155-188. The proposed messaging scope change has no equivalent guard, so a result begun before a profile switch can publish old-profile rows afterward. - Current
resolveStoredSessionis nowapps/desktop/src/app/session/hooks/use-session-actions/utils.ts:208; it still searches only$sessions, so the proposed messaging/cron fallback should be ported there.
Suggested changes
- Rework the patch at those current locations, scope cron too, and add per-operation request-generation guards.
- Add tests for active-profile and all-profiles fetches, messaging/cron owner resolution, and stale-result suppression.
Automated hermes-sweeper review.
| const refreshMessagingSessions = useCallback(async () => { | ||
| try { | ||
| const result = await listAllProfileSessions(MESSAGING_SECTION_LIMIT, 1, 'exclude', 'recent', 'all', { | ||
| const sessionProfile = profileScope === ALL_PROFILES ? 'all' : profileScope |
There was a problem hiding this comment.
Please guard this async profile-scoped fetch with a request generation counter before publishing rows. If the user switches profiles while this request is in flight, its late result can overwrite the new profile's messaging section; current main's regular-recents refresh already uses this pattern.
Current-main port validation for messaging/cron cache resolutionI hit the second symptom described by this PR on current Desktop: clicking a row already present in The current location is now: A minimal current-main port is to resolve from regular, cron, and messaging caches before performing by-ID lookup: const cached = [...$sessions.get(), ...$cronSessions.get(), ...$messagingSessions.get()].find(session =>
sessionMatchesStoredId(session, storedSessionId)
)I added a focused regression in: The test seeds a Telegram row only in
TDD evidence: the new test failed on current main because the resolver returned Validation on current
I have not opened a competing PR because this PR already contains the same underlying fallback on the pre-refactor file. I can provide the two-file current-main port as an incremental patch if useful. |
|
Resolved on main by #87566, which consolidated this PR's fix with your Co-authored-by credit preserved in the merged commits. Thank you! |
Summary
Fixes profile routing for messaging-source sessions in the Desktop sidebar.
In multi-profile/global-remote setups, messaging sessions were fetched with
profile=allregardless of the active profile. That made Telegram/other messaging sessions from one profile appear under another profile's sidebar section.Clicking one of those messaging rows could also fail to open the transcript because
resumeSession()only resolved the owning profile from the regular recents list ($sessions). Messaging rows live in$messagingSessions, so theirprofilewas dropped beforeensureGatewayProfile(),getSessionMessages(), andsession.resume.Changes
allbehavior when the sidebar is explicitly in all-profiles mode.profile,source, andlast_activein the messaging/cron section signature so profile/source changes update the atom instead of keeping stale rows.$messagingSessionsand$cronSessionsas fallbacks before resuming a stored session.Validation
npm run typecheck --workspace apps/desktoppmno longer appear in thedefaultprofile messaging section.pmprofile and hydrate the stored transcript.