fix: dismiss critic feedback widget when new events arrive (#641)#642
Draft
fix: dismiss critic feedback widget when new events arrive (#641)#642
Conversation
Dismiss pending CriticFeedbackWidget buttons when subsequent events arrive
in on_event(). Previously, the feedback buttons ('[1] Accurate', etc.)
would persist even after the agent continued working, because dismissal
only happened on user/refinement messages.
Co-authored-by: openhands <openhands@all-hands.dev>
2 tasks
Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
Coverage Report •
|
||||||||||||||||||||
Collaborator
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Core fix is sound and pragmatic, but tests violate repo guidelines.
Comment on lines
+1459
to
+1482
| def test_on_event_dismisses_pending_feedback_widgets(self, mock_cli_settings): | ||
| """on_event should dismiss pending CriticFeedbackWidgets. | ||
|
|
||
| When a new event arrives, any existing CriticFeedbackWidget should be | ||
| removed because the agent has continued working and the feedback | ||
| opportunity has passed. | ||
| """ | ||
| app: OpenHandsApp = cast(OpenHandsApp, App()) | ||
| container = VerticalScroll() | ||
| visualizer = ConversationVisualizer(container, app) | ||
|
|
||
| # Mock _dismiss_pending_feedback_widgets to track calls | ||
| visualizer._dismiss_pending_feedback_widgets = MagicMock() # type: ignore[method-assign] | ||
| # Mock _add_widget_to_ui since container isn't mounted in tests | ||
| visualizer._add_widget_to_ui = MagicMock() # type: ignore[method-assign] | ||
| # Mock _run_on_main_thread to execute immediately (we're in a test) | ||
| visualizer._run_on_main_thread = lambda func, *args: func(*args) # type: ignore[method-assign] | ||
|
|
||
| event = create_terminal_action_event("echo hello", "Say hello") | ||
|
|
||
| with mock_cli_settings(visualizer=visualizer): | ||
| visualizer.on_event(event) | ||
|
|
||
| # _dismiss_pending_feedback_widgets should have been called |
Collaborator
There was a problem hiding this comment.
🟠 Important: These tests violate the repo's testing guidelines (from AGENTS.md: "Do not use mocks in tests unless strictly necessary... You must always test real code paths in tests, NOT mocks").
Both tests mock the key functions (_dismiss_pending_feedback_widgets, _add_widget_to_ui) and only assert they were called. This doesn't actually prove the dismissal behavior works.
Suggestion: Add at least one test that exercises the real dismissal without mocks. For example:
- Create a visualizer with actual CriticFeedbackWidget instances
- Call
on_event()with a real event - Assert that the feedback widgets are actually removed from the container
The current tests verify ordering and that the call happens, which is useful, but you need at least one test that proves the real behavior works end-to-end.
Comment on lines
+1496
to
+1498
|
|
||
| call_order: list[str] = [] | ||
|
|
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: This test is better than the first one (it tracks ordering), but it still doesn't exercise real dismissal behavior. Consider keeping this for ordering verification, but add a separate test that actually creates and dismisses real widgets.
…641) The unscoped 'Button { width: 100%; }' rule in exit_modal.tcss was overriding CriticFeedbackWidget's 'Button { width: 12; }' (DEFAULT_CSS), causing only '[1] Accurate' to be visible while the other 4 buttons were pushed off-screen at full width. Fix: scope the rule to '#dialog Button' so it only applies to the exit and switch-conversation modal dialogs. Tests: - Regression test validates exit_modal.tcss has no unscoped Button rules - Snapshot test for feedback widget inside VerticalScroll (real app context) Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
…missal Replace two mock-based tests that only checked method calls with a single async integration test that: 1. Mounts a real CriticFeedbackWidget in a VerticalScroll via Textual 2. Calls on_event() with a new action event 3. Asserts the widget is actually removed from the DOM Also cleans up redundant local MagicMock imports (already at top-level). Co-authored-by: openhands <openhands@all-hands.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #641 — Two issues with the critic feedback widget:
Issue 1: Feedback buttons not dismissed when agent continues
The critic feedback buttons (
[1] Accurate,[2] Too high, etc.) persisted in the conversation history even after subsequent events/actions appeared.Root cause:
_dismiss_pending_feedback_widgets()was only called on user/refinement messages, not when new events arrived viaon_event().Fix: Added
self._run_on_main_thread(self._dismiss_pending_feedback_widgets)at the start ofon_event()inrichlog_visualizer.py.Issue 2: Only "[1] Accurate" button visible (others hidden)
Only the first button was visible because the other 4 were pushed off-screen.
Root cause: An unscoped
Button { width: 100%; height: 3; }rule inexit_modal.tcsswas overriding the feedback widget'sCriticFeedbackWidget Button { width: 12; }(defined inDEFAULT_CSS, which has lower CSS priority). Each button expanded to full width, so only the first fit.Fix: Scoped the rule to
#dialog Buttonso it only applies to the exit/switch-conversation modal dialogs.Changes
openhands_cli/tui/widgets/richlog_visualizer.py: Dismiss pending feedback widgets at the start ofon_event()openhands_cli/tui/modals/exit_modal.tcss:Button { ... }→#dialog Button { ... }tests/tui/widgets/test_richlog_visualizer.py: 2 tests for feedback dismissal on new eventstests/tui/modals/test_exit_modal.py: Regression test validating no unscopedButtonrules in exit_modal.tcsstests/snapshots/test_critic_feedback_snapshots.py: Snapshot test for feedback widget inside VerticalScroll (real app context)Verification
🚀 Try this PR