Skip to content

fix(chat): resume sessions with cursor-based history catch-up - #23

Closed
andrebrait wants to merge 3 commits into
review/resume-response-basefrom
fix/resume-response
Closed

andrebrait wants to merge 3 commits into
review/resume-response-basefrom
fix/resume-response

Conversation

@andrebrait

@andrebrait andrebrait commented Sep 13, 2026

Copy link
Copy Markdown
Owner

Companion review-only PR for kahme247#94. Base remains pinned to upstream/main a0881e1 so bot reviews cover the same diff. Do not merge this mirror.

Summary

Persisted-entry cursor catch-up for active/idle resume, bounded pages and branch/compaction reset; current partial/tool snapshots with stream epochs and selective domain ordering. Completed history is fetch-since-only while SSE keeps live output flowing. Completions during a held read queue a reread from the advanced cursor; delayed persistence comes through the existing file watcher. Earlier false-warning and rapid-send/interrupt fixes are retained.

Verification

741 tests passed, 1 existing skip; typecheck and lint passed. Explicit concurrent-completion races (persistence before/after held reply) pass. Real Chromium mobile HTTP/SSE/native-fixture smoke closes the stream, recovers entries 4/5/6 after cursor 3 without resending the prefix, restores a quiet partial without token replay, and commits the final answer once. Internal condensed adversarial review found no blocking issues.

No new dependencies or native OMP changes. Matching frontend/API deployment required; not deployed by this change.

@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 1fdcd31a-35e3-446a-be29-6948fa50cb03

📥 Commits

Reviewing files that changed from the base of the PR and between a0881e1 and b34077e.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • hooks/useAgentSession.rpc.test.mjs
  • hooks/useAgentSession.ts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The session hook now reloads saved transcript state before classifying empty agent completions. It tracks entries from before each run, preserves provider errors, avoids reusing older answers, and keeps recovery active when transcript loading fails.

Changes

Agent recovery

Layer / File(s) Summary
Session state and run boundaries
hooks/useAgentSession.ts
loadSession now returns context with agent state. New runs snapshot prior entry IDs for transcript comparisons. The mount effect uses the updated return shape.
Transcript-aware terminal recovery
hooks/useAgentSession.ts, hooks/useAgentSession.rpc.test.mjs, CHANGELOG.md
Terminal completion reloads the transcript before classifying an empty response. Recovery accepts new saved content, preserves provider errors, rejects tool-only or older answers, and remains active after transcript read failures. Tests cover lifecycle events, online reconciliation, and these response cases. The changelog records the behavior.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Browser
  participant useAgentSession
  participant SessionTranscript
  Browser->>useAgentSession: visibilitychange or agent_end
  useAgentSession->>SessionTranscript: loadSession()
  SessionTranscript-->>useAgentSession: saved assistant content or error
  useAgentSession->>useAgentSession: finishPromptWithoutStream()
Loading

Suggested reviewers: kahme247

Merge Risk: ⚪ Minimal · up to b3407

Saved-response recovery and its failure paths are covered by the updated tests. No actionable merge risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: recovering session responses through cursor-based history catch-up during resume handling.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/resume-response

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@andrebrait
andrebrait requested a lite review from Copilot September 13, 2026 09:57
@andrebrait

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copilot AI 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.

🟡 Changes recommended

A critical race can associate a new prompt with the previous run’s saved response before transcript state refreshes.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Improves recovery of saved assistant responses when terminal events are missed.

Changes:

  • Reloads transcripts before classifying empty responses.
  • Tracks entry IDs and retries failed reloads.
  • Adds regression tests and changelog documentation.
File summaries
File Summary
hooks/useAgentSession.ts Implements saved-response recovery and entry tracking; a critical stale-entry race remains.
hooks/useAgentSession.rpc.test.mjs Adds recovery and failure-path tests.
CHANGELOG.md Documents the recovery behavior.
Review details

Suppressed comments (1)

hooks/useAgentSession.ts:2396

  • The same stale-entryIds window affects abort_and_prompt: the interrupted run may already have persisted entries while the React entryIds state still reflects the pre-run transcript. If the replacement prompt is not yet persisted, recovery can treat the interrupted run's assistant response as the replacement's answer. Capture committed transcript IDs rather than this potentially stale state snapshot.
    runPreviousEntryIdsRef.current = entryIds;
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread hooks/useAgentSession.ts Outdated
@andrebrait andrebrait changed the title fix(chat): recover saved replies before empty-response errors fix(chat): resume sessions with cursor-based history catch-up Sep 13, 2026
@andrebrait

Copy link
Copy Markdown
Owner Author

Upstream kahme247#94 merged its original recovery fixes. The incremental cursor catch-up work is being moved to a new follow-up PR on the current upstream base, including the now-merged kahme247#61 behavior. Closing this review-only mirror without merging it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants