From 801321c125ec4609164c88050d8fb605fb96419a Mon Sep 17 00:00:00 2001 From: Ed_ Date: Wed, 3 Jun 2026 15:18:18 -0400 Subject: [PATCH] fix(gui): remove ListClipper from render_prior_session_view (variable-height items) ROOT CAUSE: The ListClipper in render_prior_session_view was being tripped up by the variable heights of discussion entries (huge system prompts vs small tool results). When the first entry was very tall (system prompt), the clipper would compute the visible range assuming uniform item heights, leading to underflow/overflow on subsequent items. The user saw only the first ~8 entries with massive empty space below ('early clipping'). FIX: Replace the ListClipper with a direct for loop over app.prior_disc_entries. With 233 entries, performance is acceptable and each entry renders correctly. The user can still scroll the parent imscope.child window if content overflows. Tests: - Updated test_prior_session_no_clipping.py to set entries on app_instance.controller.prior_disc_entries (the App's __getattr__ proxies attribute reads to the controller, so the set must go to the controller directly). - 28/28 broad regression pass --- src/gui_2.py | 35 +++++++++++-------------- tests/test_prior_session_no_clipping.py | 32 +++++++++------------- 2 files changed, 29 insertions(+), 38 deletions(-) diff --git a/src/gui_2.py b/src/gui_2.py index 14afc237..4716e62d 100644 --- a/src/gui_2.py +++ b/src/gui_2.py @@ -3590,25 +3590,22 @@ def render_prior_session_view(app: App) -> None: imgui.separator() avail = imgui.get_content_region_avail() with imscope.child("prior_scroll", imgui.ImVec2(avail.x, avail.y), imgui.WindowFlags_.horizontal_scrollbar): - clipper = imgui.ListClipper(); clipper.begin(len(app.prior_disc_entries)) - while clipper.step(): - for idx in range(clipper.display_start, clipper.display_end): - entry = app.prior_disc_entries[idx]; - with imscope.id(f"prior_disc_{idx}"): - collapsed = entry.get("collapsed", False) - if imgui.button("+" if collapsed else "-"): entry["collapsed"] = not collapsed - imgui.same_line(); role, ts = entry.get("role", "??"), entry.get("ts", "") - imgui.text_colored(C_LBL, f"[{role}]") - if ts: imgui.same_line(); imgui.text_colored(vec4(160, 160, 160), str(ts)) - content = entry.get("content", "") - if collapsed: - imgui.same_line(); preview = content.replace("\n", " ")[:80] - if len(content) > 80: preview += "..." - imgui.text_colored(vec4(180, 180, 180), preview) - else: - with theme.ai_text_style(): - markdown_helper.render(content, context_id=f'prior_disc_{idx}') - imgui.separator() + for idx, entry in enumerate(app.prior_disc_entries): + with imscope.id(f"prior_disc_{idx}"): + collapsed = entry.get("collapsed", False) + if imgui.button("+" if collapsed else "-"): entry["collapsed"] = not collapsed + imgui.same_line(); role, ts = entry.get("role", "??"), entry.get("ts", "") + imgui.text_colored(C_LBL, f"[{role}]") + if ts: imgui.same_line(); imgui.text_colored(vec4(160, 160, 160), str(ts)) + content = entry.get("content", "") + if collapsed: + imgui.same_line(); preview = content.replace("\n", " ")[:80] + if len(content) > 80: preview += "..." + imgui.text_colored(vec4(180, 180, 180), preview) + else: + with theme.ai_text_style(): + markdown_helper.render(content, context_id=f'prior_disc_{idx}') + imgui.separator() def render_thinking_indicator(app: App) -> None: is_thinking = app.ai_status in ['sending...', 'streaming...', 'running powershell...'] if is_thinking: diff --git a/tests/test_prior_session_no_clipping.py b/tests/test_prior_session_no_clipping.py index 821a772b..9befdfeb 100644 --- a/tests/test_prior_session_no_clipping.py +++ b/tests/test_prior_session_no_clipping.py @@ -2,7 +2,7 @@ from unittest.mock import patch, MagicMock from src.gui_2 import render_prior_session_view def test_prior_session_view_opens_scroll_child_with_explicit_size(app_instance): - app_instance.prior_disc_entries = [{"role": "User", "content": f"entry {i}", "collapsed": False, "ts": f"t{i}"} for i in range(30)] + app_instance.controller.prior_disc_entries = [{"role": "User", "content": f"entry {i}", "collapsed": False, "ts": f"t{i}"} for i in range(30)] app_instance.perf_profiling_enabled = False with patch("src.gui_2.imgui") as mock_imgui, patch("src.gui_2.imscope") as mock_imscope, patch("src.gui_2.theme") as mock_theme: _avail = type("P", (), {"x": 800.0, "y": 600.0})() @@ -23,29 +23,23 @@ def test_prior_session_view_opens_scroll_child_with_explicit_size(app_instance): mock_imgui.text_colored = MagicMock() mock_imgui.text = MagicMock() mock_imgui.separator = MagicMock() - _mock_clipper = MagicMock() - _mock_clipper.step.side_effect = [True, False] - _mock_clipper.display_start = 0 - _mock_clipper.display_end = 30 - mock_imgui.ListClipper = MagicMock(return_value=_mock_clipper) - mock_imgui.ListClipper.return_value.begin = MagicMock() mock_theme.ai_text_style = MagicMock() mock_theme.ai_text_style.return_value.__enter__ = MagicMock() mock_theme.ai_text_style.return_value.__exit__ = MagicMock() - app_instance.controller = MagicMock() try: render_prior_session_view(app_instance) except Exception as e: import pytest pytest.fail(f"render_prior_session_view raised: {e}") - assert mock_imscope.child.called, "prior_scroll child should be opened" - call_args = mock_imscope.child.call_args - args_list = call_args[0] if call_args[0] else [] - kwargs = call_args[1] if len(call_args) > 1 else {} - size_arg = args_list[1] if len(args_list) > 1 else kwargs.get("size_x", 0) - if hasattr(size_arg, 'x'): - size_x, size_y = size_arg.x, size_arg.y - else: - size_x = size_arg - size_y = kwargs.get("size_y", 0) - assert size_x == 800.0 and size_y == 600.0, f"prior_scroll should use explicit content region size, got ({size_x}, {size_y})" + assert mock_imscope.child.called, "prior_scroll child should be opened" + call_args = mock_imscope.child.call_args + args_list = call_args[0] if call_args[0] else [] + size_arg = args_list[1] if len(args_list) > 1 else 0 + if hasattr(size_arg, 'x'): + size_x, size_y = size_arg.x, size_arg.y + else: + size_x = size_arg + size_y = 0 + assert size_x == 800.0 and size_y == 600.0, f"prior_scroll should use explicit content region size, got ({size_x}, {size_y})" + id_calls = [c for c in mock_imscope.id.call_args_list] + assert len(id_calls) == 30, f"expected 30 imscope.id calls (one per entry), got {len(id_calls)}"