Skip to content

fix(desktop): hydrate pinned sessions outside recents - #74760

Closed
Rmup67350 wants to merge 2 commits into
NousResearch:mainfrom
Rmup67350:fix/her-119-pinned-session-hydration
Closed

fix(desktop): hydrate pinned sessions outside recents#74760
Rmup67350 wants to merge 2 commits into
NousResearch:mainfrom
Rmup67350:fix/her-119-pinned-session-hydration

Conversation

@Rmup67350

Copy link
Copy Markdown

Summary

  • hydrate persisted pinned sessions by ID instead of relying on the bounded recent-session page
  • preserve persisted pin order across profiles while ignoring missing or archived sessions
  • keep recent-session pagination unchanged and avoid duplicate lineage rows

Verification

  • npm run typecheck --workspace apps/desktop
  • targeted ESLint on the three changed files
  • npm run test:ui --workspace apps/desktop -- --run src/app/session/hooks/pinned-session-hydration.test.ts src/store/session.test.ts src/store/session-pin-sync.test.ts — 48 passed
  • npm run build --workspace apps/desktop
  • isolated Electron canary with 11 old pinned sessions + 60 recent fillers: 11/11 visible on first launch and 11/11 after a second full restart, without Load more

Linear: HER-119

No merge or production installation performed.

@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) area/sessions Session lifecycle, resume, persistence, history P3 Low — cosmetic, nice to have labels Jul 30, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracing the cold-start path; the current main code does leave local pins pending until a matching session row is loaded (apps/desktop/src/store/session-pin-sync.ts:122-137), so the underlying issue is real.

Problems

  • apps/desktop/src/app/session/hooks/pinned-session-hydration.ts:53 converts a real pinned:false into undefined. That bypasses the backend-authoritative unpin branch in apps/desktop/src/store/session-pin-sync.ts:84-91; reconciliation then retains the local pin and schedules writePin(..., true, ...) at lines 122-137. A stale local browser pin can therefore re-pin a session another app intentionally unpinned.
  • apps/desktop/src/app/session/hooks/use-session-list-actions.ts:189-192 omits result.messaging.sessions, although messaging sessions are pin-capable (apps/desktop/src/app/chat/sidebar/session-index.ts:6-15). A row already loaded in the messaging slice is re-fetched and appended to recents at line 215.

Suggested changes

  • Keep explicit false authoritative; use a clearly bounded migration mechanism if legacy local pins need initial precedence.
  • Include the messaging slice in the loaded-ID check and cover both cases in the hook path.

This is an automated hermes-sweeper review.

// the local pin before watchSessionPins can PATCH it true.
return {
...session,
pinned: session.pinned === true ? true : undefined,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

false is meaningful server truth: pullRemotePins() currently removes a held local pin for a loaded pinned:false row (apps/desktop/src/store/session-pin-sync.ts:84-91). Rewriting it to undefined skips that branch and lets the pending-push path write pinned:true, reasserting a pin another app may have intentionally removed. Preserve false, or gate legacy migration with explicit one-time state.

// increase to the normal recents page size.
const pinIds = $pinnedSessionIds.get()

const missingPinIds = missingPinnedSessionIds(pinIds, [

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Messaging is also a pin-capable sidebar slice, but it is omitted from this loaded-row check. A pin already present in result.messaging.sessions will be fetched again and then merged into the recents store. Include the messaging rows here or retain hydrated rows in their source slice.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
@OutThisLife

Copy link
Copy Markdown
Collaborator

Superseded by #80711. Pins past the recents page are back-filled by the list endpoint and no longer sliced off on the client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants