Skip to content

fix(desktop): preserve manual pinned session order - #66051

Open
czc6666 wants to merge 1 commit into
NousResearch:mainfrom
czc6666:fix/preserve-pinned-session-order
Open

fix(desktop): preserve manual pinned session order#66051
czc6666 wants to merge 1 commit into
NousResearch:mainfrom
czc6666:fix/preserve-pinned-session-order

Conversation

@czc6666

@czc6666 czc6666 commented Jul 17, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes pinned sessions snapping back after drag reordering in Hermes Desktop.

Two independent paths were undoing or rejecting the user's order:

  1. SidebarSessionsSection passed pinned rows through flattenSessionsWithBranches(), which sorts top-level entries by activity and therefore overwrote the persisted drag order at render time.
  2. setPinnedSessionOrder() rejected a visible reorder whenever the durable pin store also contained an unresolved or deduplicated pin that was not currently rendered.

Pinned sections now render in their already-persisted order, while regular session sections retain activity ordering. Visible pin reorders are persisted without dropping unresolved durable pins.

Related Issue

Related to #47728.

This is intentionally narrower than open PR #43661: that PR redesigns native drag-to-pin/unpin and positional drop UX, while this PR fixes the current dnd-kit reorder path's render and persistence invariants. At the latest reviewed head of #43661, both affected current-main code paths were unchanged.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Preserve the incoming persisted order when rendering the Pinned section.
  • Keep activity-based branch ordering for regular Sessions.
  • Reorder visible pinned IDs while retaining unresolved/deduplicated durable pins.
  • Ignore duplicate and unpinned IDs supplied by the rendered drag order.
  • Add five regression tests covering render ordering and persistence behavior.

How to Test

  1. Pin multiple sessions and drag an older session above a newer active session.

  2. Release the row and confirm the Pinned section keeps the chosen order instead of snapping back to activity order.

  3. Repeat while the durable pin list contains an unresolved/hidden pin; confirm visible rows reorder and the hidden pin remains stored.

  4. Run:

    npm --prefix apps/desktop run typecheck
    npm --prefix apps/desktop exec vitest run --environment jsdom src/store/layout-pinned-order.test.ts src/app/chat/sidebar/sessions-section-order.test.ts

Checklist

Code

  • I've read the Contributing Guide.
  • My commit message follows Conventional Commits.
  • I searched existing issues and PRs, including feat(desktop): refine sidebar session drag reordering #43661.
  • My PR contains only changes related to this bug fix.
  • Desktop typecheck passes.
  • I've added regression tests for the bug (5/5 pass).
  • I've tested on macOS 26.4.1 (Apple Silicon) using a packaged Hermes Desktop build.

Documentation & Housekeeping

  • Documentation update: N/A (internal behavior fix).
  • cli-config.yaml.example: N/A (no config changes).
  • CONTRIBUTING.md / AGENTS.md: N/A (no architecture/workflow changes).
  • Cross-platform impact considered: the change is pure TypeScript ordering logic with no platform-specific APIs.
  • Tool descriptions/schemas: N/A.

Screenshots / Logs

Regression verification:

Test Files  2 passed (2)
Tests       5 passed (5)
Desktop TypeScript typecheck: PASS
ESLint: PASS
Prettier: PASS
git diff --check: PASS

The fix was also verified manually in the packaged macOS Desktop app: pinned rows no longer snap back after release.

@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 17, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: LGTM / Comment

Clean fix that separates pinned session ordering (user-persisted drag order) from regular sessions (activity-sorted). New sessionEntriesForSection helper is simple and well-commented. Tests cover both order-preservation cases. No issues found.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Comment (resubmit — prior COMMENT activity noted)

Notes

  • Minor fix/feature.
  • No security concerns, no debug artifacts.
  • LGTM.

Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused fix. Current main still has the two reported failure paths: apps/desktop/src/app/chat/sidebar/sessions-section.tsx:197 sends persisted pinned rows through flattenSessionsWithBranches, whose top-level sort is recency-based at apps/desktop/src/lib/session-branch-tree.ts:95-99; and apps/desktop/src/store/layout.ts:325-332 rejects a reordered visible subset when the durable pin list contains an unresolved or deduplicated ID.

The PR's sessionEntriesForSection split and visible-order reconciliation directly address those paths while leaving normal session activity ordering intact. The added tests are behavior contracts for the two ordering invariants rather than catalog snapshots.

Automated hermes-sweeper review.

@teknium1 teknium1 added 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 18, 2026
@teknium1 teknium1 added the area/sessions Session lifecycle, resume, persistence, history label Jul 19, 2026
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