fix(desktop): persist pinned sessions across updates - #65620
fix(desktop): persist pinned sessions across updates#65620aman-merchant wants to merge 1 commit into
Conversation
ed37545 to
7886bf6
Compare
7886bf6 to
0292aa9
Compare
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Scope
- Desktop app (apps/desktop/src/app/contrib/wiring.tsx + test), +682/-2 lines
- Persists pinned sessions across updates by hydrating from localStorage on primary window mount.
Quality
- Fixes the pin-recovery edge case where secondary windows share localStorage but have independent nanostore instances.
- Only primary window triggers hydration to avoid conflicts.
- New getDesktopPinnedSessions / saveDesktopPinnedSessions test coverage.
Looks Good
- Clean desktop-specific fix with proper window-role awareness.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the careful recovery and write-ordering work. The current localStorage-only pin atom on main (apps/desktop/src/store/layout.ts:72) confirms the reported persistence gap.
Problems
- The new helpers omit
profile(apps/desktop/src/hermes.ts:487,:493). Electron consequently routes them through the primary backend (apps/desktop/electron/main.ts:7966-7980); in global remote mode that is the remote host (apps/desktop/electron/main.ts:6111-6149). The proposed endpoint writes that host's default Hermes root (hermes_cli/web_server.py:5407-5425), so this is backend-global rather than machine-global and can mix pin lists from distinct Desktop clients. - The new state tests mock the API boundary (
apps/desktop/src/store/layout-pinned-session-sync.test.ts:8-10), leaving this routing/ownership path untested.
Suggested changes
- Store machine-owned pin recovery state through an Electron IPC/user-data capability, or explicitly adopt and test a backend-owned scope.
- Cover both local and global-remote routing before merging.
Automated hermes-sweeper review.
|
|
||
| export function getDesktopPinnedSessions(): Promise<DesktopPinnedSessionsState> { | ||
| return window.hermesDesktop.api<DesktopPinnedSessionsState>({ | ||
| path: '/api/desktop/pinned-sessions' |
There was a problem hiding this comment.
This unprofiled call is routed by Electron through the primary backend; in global remote mode that backend is the remote host. Combined with the new server-side default-root record, pins become remote/backend-global rather than machine-local. Please persist this through an Electron-owned IPC store, or explicitly define and test the intended backend scope.
|
Superseded by #74234, which makes pins server-owned so they survive paging and sync between apps. Your work is carried in it and you're credited as a co-author — thank you. |
Summary
Design
Pins remain machine-global rather than following the active Hermes profile. This preserves the existing Desktop behavior and the All profiles view while ensuring profile switches cannot replace the pin set.
The backend record is canonical only when the renderer has no unsaved local change. A local dirty marker survives renderer restart, retries a failed final PUT, and prevents stale durable data from overwriting a newer local list. Mutations made while the recovery GET or first migration PUT is pending are reconciled and serialized rather than dropped.
Verification
scripts/run_tests.sh tests/hermes_cli/test_desktop_ui_state.py tests/hermes_cli/test_web_server_profile_unification.py -q— 37 passednpm --workspace apps/desktop run test -- --run src/lib/pinned-session-state.test.ts src/store/layout-pinned-session-sync.test.ts src/hermes-profile-scope.test.ts— 16 passednpm --workspace apps/desktop run typecheck— passednpm --workspace apps/desktop run lint— passed (15 pre-existing warnings, 0 errors)npm --workspace apps/desktop run build— passedThe focused state-machine tests cover mutation during the recovery GET, mutation during the initial migration PUT, transient and exhausted final-write failures, restart with a dirty local record, intentional empty state, StrictMode duplicate hydration, and exclusion of secondary session windows from all pin recovery writes.
A repository-wide Python run was also attempted with the shared installed venv, but that environment lacks
pytest-asyncio; unrelated ACP/async tests failed at collection/runtime, so the run was stopped after 4.5%. All changed-file suites above are green.