fix(desktop): scope messaging and cron session fetches to active profile - #60688
fix(desktop): scope messaging and cron session fetches to active profile#60688Ahmett101 wants to merge 2 commits into
Conversation
refreshCronSessions, refreshMessagingSessions, and loadMoreMessagingForPlatform all hardcoded profile='all' in their listAllProfileSessions() calls, causing sessions from all profiles to leak into the sidebar regardless of which profile is active. Apply the same profileScope resolution pattern already used by refreshSessions to the other three functions, and add profileScope to their useCallback dependency arrays. Closes NousResearch#60678
Related: #52910 (broader competing PR — same fix relocated into |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for narrowing this to the extracted session-list hook. The premise is confirmed on current main: apps/desktop/src/app/session/hooks/use-session-list-actions.ts:86, :102, and :126 still pass 'all', while Recents resolves profileScope at :174.
Problems
- The new profile-specific callbacks need stale-response protection.
refreshSessionsinvokes them after its own awaited request (use-session-list-actions.ts:191-193); a prior-profile closure may therefore start a late fetch after a switch. Recents guards its writes withrefreshSessionsRequestRef(:180-188), but the cron and messaging setters do not (:90,:110-113). - Please add regression coverage for concrete-profile and
ALL_PROFILESarguments across all three changed fetch paths. Current helper coverage only asserts the default all-profile request (apps/desktop/src/hermes.test.ts:51-60).
Suggested changes
- Discard stale cron/messaging responses with a request generation or current-scope check.
- Add a hook-level test covering profile changes and a delayed old response.
Automated hermes-sweeper review.
| try { | ||
| const result = await listAllProfileSessions(MESSAGING_SECTION_LIMIT, 1, 'exclude', 'recent', 'all', { | ||
| const sessionProfile = profileScope === ALL_PROFILES ? 'all' : profileScope | ||
| const result = await listAllProfileSessions(MESSAGING_SECTION_LIMIT, 1, 'exclude', 'recent', sessionProfile, { |
There was a problem hiding this comment.
This fetch is now profile-specific, but it has no generation/current-scope guard. refreshSessions can invoke an old closure after awaiting its own request (use-session-list-actions.ts:191-193), so a late previous-profile response can overwrite the new profile's messaging rows. Please discard stale responses here and in the analogous cron/pagination paths.
|
Addressing @teknium1's review comment: Added per-function diff --git a/apps/desktop/src/app/session/hooks/use-session-list-actions.ts b/apps/desktop/src/app/session/hooks/use-session-list-actions.ts
index 373618ad..63295faf 100644
--- a/apps/desktop/src/app/session/hooks/use-session-list-actions.ts
+++ b/apps/desktop/src/app/session/hooks/use-session-list-actions.ts
@@ -77,6 +77,9 @@ export function useSessionListActions({ profileScope }: UseSessionListActionsArg
export function useSessionListActions({ profileScope }: UseSessionListActionsArgs) {
const refreshSessionsRequestRef = useRef(0)
+ const cronSessionsRequestRef = useRef(0)
+ const messagingSessionsRequestRef = useRef(0)
+ const loadMoreMessagingRequestRef = useRef(0)
const refreshCronSessions = useCallback(async () => {
+ const requestId = cronSessionsRequestRef.current + 1
+ cronSessionsRequestRef.current = requestId
try {
const sessionProfile = profileScope === ALL_PROFILES ? 'all' : profileScope
const { sessions } = await listAllProfileSessions(...)
- setCronSessions(prev => (sameCronSignature(prev, sessions) ? prev : sessions))
+ if (cronSessionsRequestRef.current === requestId) {
+ setCronSessions(prev => (sameCronSignature(prev, sessions) ? prev : sessions))
+ }
} catch { }
const refreshMessagingSessions = useCallback(async () => {
+ const requestId = messagingSessionsRequestRef.current + 1
+ messagingSessionsRequestRef.current = requestId
try {
...
- setMessagingSessions(...)
- setMessagingTruncated(...)
+ if (messagingSessionsRequestRef.current === requestId) {
+ setMessagingSessions(...)
+ setMessagingTruncated(...)
+ }
} catch { }
const loadMoreMessagingForPlatform = useCallback(async (platform) => {
+ const requestId = loadMoreMessagingRequestRef.current + 1
+ loadMoreMessagingRequestRef.current = requestId
...
const result = await listAllProfileSessions(...)
+ if (loadMoreMessagingRequestRef.current !== requestId) return
...
})Full patch available at: CaptainHowlingMadMurdockBot@63295fa The branch TypeScript typecheck ( |
|
Status update: the desktop cron helper scoping landed via #67493/#67602 and list filtering via #67615, but this PR's target — scoping the messaging/cron session fetches in use-session-list-actions — is a different call-site set that those merges don't cover (except the cron-jobs refresh, which #67615 now scopes). Leaving open; a rebase onto current main narrowing to the session-fetch call sites would make this reviewable. |
|
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
When using the Hermes Desktop with multiple profiles, the Messaging and Cron sidebar sections leaked sessions from all profiles regardless of which profile was active. The main Recents list correctly scoped to the active profile, but
refreshCronSessions,refreshMessagingSessions, andloadMoreMessagingForPlatformall hardcodedprofile='all'.Changes
apps/desktop/src/app/session/hooks/use-session-list-actions.ts: Applied the sameprofileScope === ALL_PROFILES ? 'all' : profileScoperesolution pattern already used byrefreshSessionsto the other three functions. AddedprofileScopeto theiruseCallbackdependency arrays.How to Test
Manual: Switch profiles in Desktop App (Ctrl+D) and verify Messaging/Cron sidebar sections only show sessions from the active profile, not all profiles.
Checklist
Closes #60678