Skip to content

Render conversation history in the UI on resume/switch#734

Open
cbagwell wants to merge 3 commits into
OpenHands:mainfrom
cbagwell:feat/history-on-restore
Open

Render conversation history in the UI on resume/switch#734
cbagwell wants to merge 3 commits into
OpenHands:mainfrom
cbagwell:feat/history-on-restore

Conversation

@cbagwell

Copy link
Copy Markdown
Contributor

Summary

  • Replay up to 50 events when resuming/switching conversations
  • Support incrementing loading of additional events on demand

This is a continuation of changes proposed in PR #602.

Possible future improvement: Support infinite scrolling style auto-loading of additional events.

Demo Video

Screen.Recording.2026-05-10.at.3.27.37.PM.mov

Fixes

#287

@all-hands-bot all-hands-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Acceptable - The history replay feature works well, but there's a critical issue with duplicate side effects that must be fixed before merging.

Comment thread openhands_cli/tui/widgets/richlog_visualizer.py
Comment thread openhands_cli/tui/widgets/richlog_visualizer.py
Comment thread openhands_cli/tui/widgets/richlog_visualizer.py
@all-hands-bot

Copy link
Copy Markdown
Collaborator

Detailed Code Review Analysis

[CRITICAL ISSUES] (Must fix)

  • [openhands_cli/tui/widgets/richlog_visualizer.py, Lines 464-471] Side Effect Duplication: During history replay, _emit_critic_result_side_effects() is called for every historic critic result, which re-sends PostHog analytics events and re-posts CriticResultReceived messages. This pollutes analytics data and could trigger duplicate refinement flows. Side effects should only fire when critic results are first received, not during replay.

[IMPROVEMENT OPPORTUNITIES] (Should fix)

  • [openhands_cli/tui/widgets/richlog_visualizer.py, Line 136] Type Annotation: Add type annotation for _load_more_widget: Static | None = None for consistency with other attributes.

  • [tests/snapshots/test_visualizer_snapshots.py] Test Coverage: Consider adding a test that actually clicks the "load more" banner and verifies that additional events are loaded correctly. Current snapshot tests only verify the banner rendering, not the interactive behavior.

[TESTING GAPS]

  • No test verifying that side effects (PostHog events, CriticResultReceived messages) are NOT duplicated when replaying history. This is the main bug found in this review.
  • No interactive test for the load-more functionality (clicking the banner, verifying pagination).

[RISK ASSESSMENT]

  • Overall PR: ⚠️ Risk Assessment: 🟡 MEDIUM

The history replay feature is well-designed with good pagination and UI/UX considerations. However, the duplicate side effect emission during replay is a critical bug that could:

  1. Analytics pollution: Every resume/switch will re-send PostHog events for all historic critic results, making analytics data unreliable
  2. Duplicate message processing: Re-posting CriticResultReceived messages could trigger refinement flows multiple times if handlers aren't idempotent
  3. Performance impact: For conversations with many critic results, replaying could trigger significant side effect processing

Risk Factors:

  • Affects core conversation resume/switch flow (high user impact)
  • Side effects involve external systems (PostHog) and internal message bus
  • The refactoring that exposed this bug (_build_critic_widgets + _emit_critic_result_side_effects split) is otherwise good design

Recommendation: Fix the side effect duplication before merging. Add a flag or parameter to skip side effects during replay, or refactor _emit_critic_result_side_effects() to check if the event is from replay context.


VERDICT:
Needs rework: Fix the critical side effect duplication issue before merging.

KEY INSIGHT:
The refactoring of critic result handling into separate widget-building and side-effect methods is good separation of concerns, but it exposed a critical bug where history replay re-emits all side effects. The fix is straightforward: skip _emit_critic_result_side_effects() during replay and only render widgets.


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.

@cbagwell

Copy link
Copy Markdown
Contributor Author

Review comments addressed.

cbagwell and others added 3 commits May 13, 2026 21:44
Replays the last 50 persisted events when resuming a conversation
via --resume or switching via /history panel.

Co-authored-by: Jathin Sreenivas <sjathin@amazon.com>
Shows a clickable banner indicating earlier events can be loaded
and loads more when clicked.
@cbagwell cbagwell force-pushed the feat/history-on-restore branch from 4f64e85 to 79afaf3 Compare May 14, 2026 02:53
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