fix(desktop): preserve session list on fetch failure instead of dropping to empty - #64159
fix(desktop): preserve session list on fetch failure instead of dropping to empty#64159DavidMetcalfe wants to merge 2 commits into
Conversation
…ing to empty refreshSessions() used try/finally with no catch block. When listAllProfileSessions() threw (backend timeout on cold start after update, network error), $sessions stayed at [] (initial atom value) and $sessionsLoading flipped to false — the sidebar rendered "no sessions" with zero error indication. On first boot after an update this looks exactly like data loss. Add a catch block that preserves the previous session list and logs the error to console for debugging. The spinner still stops (finally), but the sidebar keeps whatever sessions were loaded before the failure.
c5a23e4 to
8b572b6
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the failure path. The current change needs adjustment before it can deliver the stated UX guarantee.
Problems
refreshSessions()already leaves an existing list intact on rejection:setSessionsis only reached after a successful result atapps/desktop/src/app/session/hooks/use-session-list-actions.ts:175-182. The addedcatchdoes not write session state, while$sessionsbegins as[]atapps/desktop/src/store/session.ts:215; therefore it cannot preserve a list during the reported cold-start failure.- The catch also suppresses the boot failure path. Boot awaits
refreshSessions()atapps/desktop/src/app/gateway/hooks/use-gateway-boot.ts:479-485; a rejection currently reachesfailDesktopBoot/notifyErrorat lines 487-492. With this catch, boot completes while the initial list remains empty.
Suggested changes
- Keep an initial fetch failure observable (for example, an explicit recoverable session-load error) rather than treating it as a successful empty result, while retaining stale rows after a later refresh failure.
- Add rejection-path coverage for both an already-populated list and the initial empty state.
Automated hermes-sweeper review.
| @@ -182,6 +182,13 @@ export function useSessionListActions({ profileScope }: UseSessionListActionsArg | |||
| setSessionsTotal(typeof result.total === 'number' ? result.total : result.sessions.length) | |||
| setSessionProfileTotals(result.profile_totals ?? {}) | |||
| } | |||
There was a problem hiding this comment.
This catch resolves the initial fetch failure without changing $sessions. Since $sessions starts as [], boot then reaches completeDesktopBoot() (use-gateway-boot.ts:479-485) with an empty sidebar instead of the existing boot error path (use-gateway-boot.ts:487-492). Please keep an initial failure observable or provide a recoverable error/retry state rather than swallowing it here.
There was a problem hiding this comment.
Thanks @teknium1 — both findings were correct. The original catch preserved $sessions only when there was something to preserve, so cold-start boot ended up with an empty sidebar and no error path because the rejection was swallowed before the existing boot error handler could see it. Pushed a rework at 3f83bfcb4 that:
- Introduces
$sessionLoadError(a typedSessionLoadErroratom instore/session.ts) plus aSessionRefreshErrorclass so the boot hook can tell a session-list failure apart from other boot failures. - Adds an
isBoot?: booleanoption torefreshSessions(). The boot caller passesisBoot: trueand the catch re-throws asSessionRefreshError, which keeps the existingfailDesktopBoot+notifyErrorpath intact. Background callers (sidebar refresh, profile switch, reconnect) don't pass the flag and resolve normally with$sessionLoadErrorset, so their stale rows stay visible. - Clears
$sessionLoadErrorinside the existing success block — a transient success auto-clears the banner without a Retry click. - Adds
boot.errors.sessionLoadFailed(en/ja/zh/zh-hant) so the toast/overlay says "Couldn't load recent sessions" instead of the generic "Desktop boot failed" copy. - 5 rejection-path tests in a new
use-session-list-actions.test.tsx: cold-boot re-throws asSessionRefreshError, populated-list refresh failure preserves rows + records the error, success-after-failure clears the atom, non-Errorrejection values are handled, and the explicit-isBootcontract is pinned (so future refactors don't start using$sessions.length === 0as a substitute).
The "couldn't refresh — Retry" affordance in the sidebar UI is intentionally out of scope here — $sessionLoadError is now exposed so a follow-up PR can wire it up without re-touching this state machine. Happy to take that as a sibling if you'd rather have it land together.
Cross-vendor dual review (Flash + GPT-OSS) on the rework landed without BLOCKERs; the consensus was an explicit isBoot flag over a $hasFetchedSessions atom, plus the typed error class and the session-specific i18n key.
Address maintainer review on NousResearch#64159 (review #4709792830): The original change added a silent catch to refreshSessions(), but on cold start $sessions is [] (store/session.ts:217) and setSessions is only reached inside the success block (use-session-list-actions.ts:178-182), so the catch preserved nothing. It also re-threw nothing, which meant the existing boot failure path at use-gateway-boot.ts:486-492 (failDesktopBoot + notifyError) never fired — boot silently completed with an empty sidebar and no error UI. Rework: * Add $sessionLoadError atom (store/session.ts) so the sidebar can surface a recoverable error after a populated-list refresh failure without dropping existing rows. * Add SessionRefreshError class + { isBoot?: boolean } option to refreshSessions. The boot caller passes isBoot:true so cold-start failures re-throw and reach the existing boot overlay; background callers resolve normally with the error recorded. * clearSessionLoadError() on every successful setSessions write so a transient success auto-clears the banner without a Retry click. * Add boot.errors.sessionLoadFailed i18n key (en/ja/zh/zh-hant) so the boot overlay tells the user it was the session fetch, not the whole desktop boot. Generic copy is preserved for all other boot failures. * Rejection-path coverage: 5 tests in use-session-list-actions.test.tsx covering cold-boot re-throw, populated-list preservation, success-clears-error, non-Error rejection values, and the explicit-isBoot-flag contract. Closes the inline review (#4709792830 / comment id 3592083182).
Summary
Fixes #64157:
refreshSessions()silently drops the session list to empty on transient fetch failures, making the sidebar appear as if all sessions are gone.What this PR does
Adds a
catchblock torefreshSessions()inapps/desktop/src/app/session/hooks/use-session-list-actions.ts. Previously the function usedtry/finallywith nocatch— whenlistAllProfileSessions()threw (backend timeout on cold start after update, network error),$sessionsstayed at its initial atom value of[]and$sessionsLoadingflipped tofalse. The sidebar rendered "no sessions" with zero error indication.The catch block preserves the previous session list on transient failure. The spinner still stops (finally), but the sidebar keeps whatever was loaded before the failure instead of dropping to empty.
Notes
active-profile.jsonmigration (separate issue) compounds this — on first boot after update, the Desktop defaults to the "default" profile, and if that profile's session fetch times out, the user sees an empty sidebar and assumes everything is gone.