Skip to content

fix(desktop): recover stored session transcripts during resume - #83802

Closed
friendfish wants to merge 14 commits into
NousResearch:mainfrom
friendfish:codex/fix-83729
Closed

friendfish wants to merge 14 commits into
NousResearch:mainfrom
friendfish:codex/fix-83729

Conversation

@friendfish

Copy link
Copy Markdown

Summary

  • paint a valid REST transcript as soon as it arrives instead of holding it behind a pending session.resume RPC
  • treat omitted messages as unavailable rather than authoritative empty history, recover by the resume-bound stored identity, and arm existing retry UI from authoritative message_count metadata
  • preserve optimistic, streaming, live-projection, and journal rows while enforcing the existing large-session paint budget and foreground array reuse

Testing

  • npx vitest run src/app/session/hooks/use-session-actions.test.tsx (51 tests)
  • npm run typecheck
  • npm run lint -- --quiet
  • npm run build
  • npx playwright test e2e/large-session-resume.spec.ts --grep "cold resume" (2 tests)
  • mutation check: an unconditional settle-time $messages clone fails the new reference-identity regression assertion

Fixes #83729

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 11, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

fix(desktop): recover stored session transcripts during resume

  1. Exact paint-count assertion is timing-sensitive — apps/desktop/e2e/large-session-resume.spec.ts (assertUnchangedResume(..., { bursts: 1, kind: 'exact' }) → expect(paints.bursts).toBe(1)): the paint observer uses a 30ms flush debounce and the test waits a fixed waitForTimeout(1_000). On a slow CI runner the timer can coalesce or split bursts, making an exact equality flaky. The maximum budget path (toBeGreaterThanOrEqual(1) + toBeLessThanOrEqual) is far more forgiving; consider asserting a >= 1 lower bound alongside the exact upper bound, or polling the burst count with waitFor instead of a fixed sleep.

  2. Fallback REST call passes an unverified profile argument — apps/desktop/src/app/session/hooks/use-session-actions/index.ts (getLatestSessionMessages(resumedStoredSessionId || storedSessionId, sessionProfile)): the new hook tests assert the fallback is invoked as ('stored-1', undefined), meaning sessionProfile is undefined in the harness. Confirm the production path passes the same profile to both the initial prefetch and the fallback; if they can differ, a profile-scoped gateway could hydrate the retry path from the wrong store.

  3. Latch interplay with ancestor-prefix transcripts is untested — use-session-actions/index.ts (responseClaimsHistory = !createdThisRun.has(storedSessionId) && (resumed.message_count ?? 0) > 0): the design doc explicitly notes some gateway branches can have zero raw rows while the visible transcript comes from an ancestor prefix, and that case leans on the sidebar row as the other side of the OR. There is no test for "sidebar row absent + message_count > 0 + transcript legitimately from an ancestor prefix" — that combination would arm the failure latch and blank a valid session. An explicit test for the latch NOT arming there would pin the documented behavior.

  4. JSON.stringify-based assertions are brittle — apps/desktop/src/app/session/hooks/use-session-actions.test.tsx (expect(JSON.stringify($messages.get())).toContain('persisted question')): string-containment on serialized message arrays is sensitive to key order/whitespace and only proves a substring exists. The file already uses this pattern elsewhere, but for the new tests a small helper that locates a message by content would make failures far easier to diagnose.

@OutThisLife

Copy link
Copy Markdown

Thanks @friendfish. Closing as superseded: main now paints the prefetched REST transcript before resume settles and latches the stale-list-row case (ad800ea, f454288), which covers what this PR set out to do. #83729 is closed as fixed.

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

Labels

comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists 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.

[Bug]: Desktop — stored session opens as a silent blank thread after restart (backend intact; New Window recovers)

4 participants