Lazily load conversation history: 50 most recent first, paginate on s…

[rbren]

…croll

Previously every conversation page loaded its entire event history in a single REST call (limit: 100 plus a WebSocket resend_all replay), which is slow and wasteful for long-running conversations.

Now:

• EventService.searchEvents takes an options bag (limit, pageId, sortOrder, timestampGte, timestampLt) and returns the raw EventPage so callers can paginate. Both local and cloud-proxy paths forward the params.
• useConversationHistory fetches only the most recent INITIAL_HISTORY_PAGE_SIZE (50) events with sort_order='TIMESTAMP_DESC' and reverses them to chronological order.
• useLoadOlderEvents is a new on-demand hook that paginates older events via timestamp__lt = <oldest known timestamp>. ChatInterface triggers it when the user scrolls within 80px of the top, shows a loading spinner, and preserves the visible scroll position by adjusting scrollTop by the inserted height delta.
• ConversationWebSocketProvider waits for the REST query to settle before opening the main socket, then subscribes with resend_mode='since' and after_timestamp = <latest preloaded event timestamp>, falling back to resend_mode='all' if REST returned no events or errored. The legacy count-based history loading detection is dropped for the main connection.
• Event store gained a bulk addEvents action (used for the initial REST seed and for "scroll-up" pagination) that re-sorts by timestamp once at the end so prepending an older page stays O(N log N) overall.

• A human has tested these changes.

---

Why

Summary

Issue Number

How to Test

Video/Screenshots

Type

• Bug fix
• Feature
• Refactor
• Breaking change
• Docs / chore

Notes

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agent-canvas	Ready	Preview, Comment	May 10, 2026 7:02pm

[all-hands-bot]

Well-structured pagination implementation with good test coverage. A few issues to address around error handling, plus this should go through eval/QA since it changes conversation loading behavior.

[all-hands-bot]

Code Review Summary

Taste Rating: 🟡 Acceptable - Good architectural approach with solid test coverage, but needs error handling improvements.

---

What Works Well

✅ Smart pagination strategy: Loading only the 50 most recent events first is pragmatic - solves the real problem of slow loads for long conversations without over-engineering.

✅ REST-first, then WebSocket: Waiting for the initial REST query to settle before opening the WebSocket with resend_mode='since' is elegant - avoids re-streaming history we already have and handles the synchronization cleanly.

✅ Comprehensive tests: New tests for useLoadOlderEvents, updated tests for useConversationHistory, and store tests for bulk operations. Good coverage of the happy path and edge cases like ordering.

✅ Scroll position preservation: The scrollHeight delta calculation is the standard pattern for prepending content without jarring the user.

✅ Deduplication: Event store's eventIds set handles overlap between REST and WebSocket gracefully.

---

Issues Found

See inline comments for details:

1. 🟠 Important - Silent error handling in pagination (chat-interface.tsx:148)
2. 🟡 Suggestion - Performance: duplicate sorting in bulk insert (use-event-store.ts:115)
3. 🟡 Suggestion - Missing user feedback for timestamp edge case (use-load-older-events.ts:48)

---

Risk Assessment

⚠️ Risk Level: 🟡 MEDIUM

Key Risk Factors:

• Behavior Change: This PR changes how conversation history is loaded (REST + pagination vs. full WebSocket replay), which affects conversation initialization timing and event ordering guarantees.
• WebSocket Synchronization: The change from resend_all to resend_mode='since' with after_timestamp alters the contract between client and server for history replay.
• User-Facing: Any bugs in pagination or scroll handling directly impact the conversation UX.

Mitigating Factors:

• Well-tested with dedicated test cases for pagination, ordering, and bulk operations
• Error states are handled (REST query errors fall back to WS resend_mode='all')
• Deduplication prevents event duplication even if REST/WS overlap

Recommendation:

• Fix the error handling gap (comment Add sandbox project metadata #1) before merging - silent pagination failures are confusing.
• Run through eval/QA - per repo guidelines, changes to conversation behavior should go through lightweight evals to catch any regressions in event ordering, message display, or agent interaction patterns.
• Consider performance optimization (comment Prototype npm-package agent-server-gui for OpenHands reuse #2) if you see slow rendering with large event batches in production, but not blocking.

---

Verdict

❌ Needs rework: Fix error handling for pagination failures, then flag for human review + eval before merging.

Key Insight: The pagination architecture is sound, but production reliability requires proper error surfacing. Users need to know when "scroll up for more" fails, not just see a spinner disappear.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Error handling for pagination is intentionally minimal because..."). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

[rbren]

@OpenHands address any PR feedback that has not already been addressed. resolve comments once complete

[openhands-ai]

I'm on it! rbren can track my progress at all-hands.dev

[all-hands-bot]

Well-structured lazy loading implementation with good test coverage, but the three unresolved issues from the previous review are still present and should be addressed before merging.

[all-hands-bot]

Code Review Summary

Taste Rating: 🟡 Acceptable - Core architecture is solid but has implementation gaps

---

Architecture Analysis

What's Good:

• Smart architectural shift from WebSocket-first to REST-first loading
• Proper separation of concerns: useConversationHistory for initial load, useLoadOlderEvents for pagination
• WebSocket waits for REST to finish and uses resend_mode='since' to avoid duplicates
• Comprehensive test coverage for the new pagination behavior
• Scroll position preservation logic is well-thought-out

Data Flow:

1. REST loads most recent 50 events (TIMESTAMP_DESC, reversed to chronological)
2. WebSocket connects with after_timestamp to only get new events
3. User scrolls up → triggers loadOlder() with timestamp__lt for pagination
4. Clean deduplication via eventIds Set

---

[IMPROVEMENT OPPORTUNITIES]

The three unresolved issues from the previous review are still present:

1. Silent error handling (chat-interface.tsx:147)

   • Empty catch block claims errors surface via "global toast / banner" but no code actually does this
   • Users see loading spinner disappear with no explanation when pagination fails
   • Fix: Call setErrorMessage() in the catch block

2. No user feedback for missing timestamp (use-load-older-events.ts:48)

   • Silently returns when oldest event has no timestamp
   • User scrolls up expecting events, nothing happens, no explanation
   • Fix: Set hasMore = false or show a toast

3. Inefficient sorting (use-event-store.ts:104-115)

   • addEvents calls applyAddEvent in a loop (which may sort each iteration), then sorts again unconditionally
   • For a 50-event page, this could sort 51 times instead of once
   • Fix: Add a flag to skip intermediate sorts, or implement addEvents without the loop

4. Breaking API change (event-service.api.ts:95-99)

   • searchEvents signature changed: limit moved from positional parameter to options object
   • Will break external callers using the old signature
   • Fix: Document as breaking change or support both signatures temporarily

---

[TESTING GAPS]

✅ Good coverage:

• Scroll-up pagination triggers correctly
• Agent events render without user messages (fixes blank chat bug)
• Chronological ordering despite server returning TIMESTAMP_DESC
• Bulk add deduplication
• hasMore detection

No additional tests needed.

---

[RISK ASSESSMENT]

⚠️ Risk Level: 🟡 MEDIUM

Risk Factors:

• Scope: Significant architectural change to conversation loading
• User Impact: Affects all conversation views; poor error handling could confuse users
• Performance: Inefficient sorting is suboptimal but not critical for 50-event pages
• Breaking Change: API signature change could break library consumers

Recommendation: Address the error handling gaps (#1, #2) before merging. The performance issue (#3) is worth fixing but not blocking. The breaking change (#4) should be documented in the PR description.

---

VERDICT:
❌ Needs rework: The silent error handling and missing user feedback issues should be fixed before merging. The core lazy loading architecture is solid and well-tested.

KEY INSIGHT:
The REST-first + WebSocket-since pattern is the right architectural move for scalability, but user-facing error handling needs to match the sophistication of the data layer.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

[openhands-ai]

OpenHands encountered an error: Request timeout after 30 seconds to https://tjzurqazzqttrice.prod-runtime.all-hands.dev/api/conversations/bb3c2b7f-974e-48ee-9224-9a7d3852459d/ask_agent

See the conversation for more information.

[all-hands-bot]

Well-structured lazy loading implementation with good test coverage. However, there are critical race conditions and performance issues that must be fixed before merging. Additionally, this PR changes conversation loading behavior (agent runtime behavior) and requires lightweight eval/QA before approval per repo policy.

[all-hands-bot]

Risk Assessment

⚠️ Risk Level: 🟡 MEDIUM

Risk Factors:

• Behavior Change (Eval Risk): Changes conversation loading behavior and event streaming, which could affect agent runtime performance and benchmarks. Per repo policy, this requires lightweight eval/QA before approval.
• Performance Impact: The lazy loading approach should improve initial load time for long conversations, but the O(N×M) complexity issue in addEvents could degrade scroll-up performance.
• Race Conditions: Multiple identified race conditions (concurrent loadOlder() calls, REST→WS handoff gap) could cause duplicate requests or missed events.
• State Management: Infinite loading state on REST error could leave users stuck with no recovery path.

Recommendation: Do not auto-merge. After addressing the critical issues:

1. Run lightweight benchmarks comparing conversation load times before/after
2. Test with long conversations (1000+ events) to verify scroll-up performance
3. Test error recovery paths (network failures, server errors)
4. Verify no eval regressions on standard agent tasks

---

Key Architectural Insight

Data structure choice is sound - The REST-first + WS-since pattern with timestamp-based pagination elegantly solves the "load everything upfront" problem. However, the execution has concurrency gaps: the state-based guards (isLoading, hasMore) rely on React's async state updates instead of synchronous refs, creating windows where multiple triggers can fire. The addEvents bulk operation correctly deduplicates and sorts once, but the implementation still creates O(N×M) intermediate copies. Fix the ref-based atomicity and Set accumulation, and this becomes a clean, efficient solution.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

[rbren]

@OpenHands address the PR feedback and fix the failing gh action

[openhands-ai]

I'm on it! rbren can track my progress at all-hands.dev

[all-hands-bot]

Well-structured lazy loading implementation with good test coverage. However, four unresolved issues from the previous review remain and should be addressed before merging.

The core architecture is solid: REST-first for the most recent 50 events, then WebSocket with resend_mode='since', and on-demand pagination via scroll. The bulk event insertion optimization and ref-based race guards are good improvements.

Key issues to fix:

1. Missing API response validation - could cause runtime errors if server returns malformed data
2. Inconsistent hasMore detection - different logic between initial load and pagination
3. Potential scroll jank - position restoration may read stale DOM height
4. Inefficient refetch handling - minor performance concern

See inline comments for details and suggested fixes.

[all-hands-bot]

[RISK ASSESSMENT]

⚠️ Risk Assessment: 🟡 MEDIUM

This PR changes the core conversation loading flow from a single blocking REST call + WebSocket replay to a paginated REST-first approach. Key risk factors:

• Missing input validation could cause runtime TypeError if the API returns malformed responses ({items: null})
• Inconsistent pagination logic between initial load and scroll-up may cause UI state bugs with certain agent servers
• Potential scroll jank from reading potentially-stale DOM heights during batch updates
• Frontend-only change - does NOT affect agent runtime behavior, so no eval/QA required despite previous review comments

The unresolved validation issues elevate this from LOW to MEDIUM risk. They should be fixed before merging to prevent production conversation-loading failures.

---

VERDICT:

❌ Needs fixes: Address the three Important-level validation/consistency issues before merging. The scroll jank and refetch efficiency issues are lower priority but worth fixing.

KEY INSIGHT:

Solid lazy-loading architecture with good performance optimizations (bulk insert, ref-based race guards), but defensive programming is missing at the API boundary - always validate external data before spreading/iterating.

[openhands-ai]

OpenHands encountered an error: Request timeout after 30 seconds to https://lrbrjkifgcmbzrwy.prod-runtime.all-hands.dev/api/conversations/700bdfac-afe9-4ce3-97c1-4b71756b386e/ask_agent

See the conversation for more information.

[rbren]

Addressed the latest feedback in 66dbcc1.

Changes:

• Added defensive Array.isArray(page.items) validation in both useConversationHistory() and useLoadOlderEvents() so malformed responses fail with clear errors instead of crashing during reversal.
• Made the initial history hook use the same pagination fallback heuristic as scroll-up loading: a full first page now keeps hasMore=true even if an older server omits next_page_id.
• Added regression coverage for malformed responses and the full-page/no-next_page_id case.

Verification:

• npm run lint
• npm test
• npm run build
• GitHub CI is green again on the updated commit.

This comment was generated by an AI agent (OpenHands) on behalf of the user.

[all-hands-bot]

🟢 Good taste - Clean lazy loading implementation with solid REST-first → WebSocket pattern. No technical issues found.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM

Changes conversation loading mechanism (REST-first with 50-event window, then WebSocket resend_mode='since') and adds scroll-based pagination. However, this is a frontend performance optimization that doesn't affect agent behavior—the agent runtime still receives all events via WebSocket as before. Good test coverage and error handling throughout.

VERDICT:
✅ Worth merging: Well-designed lazy loading that improves frontend performance without changing agent execution logic.

KEY INSIGHT:
The REST-first pattern with after_timestamp subscription elegantly avoids duplicate event streaming while maintaining real-time updates.