diff --git a/clients/terminal/tui/widgets/chat_panel.py b/clients/terminal/tui/widgets/chat_panel.py index 1bf27b4..33ab039 100644 --- a/clients/terminal/tui/widgets/chat_panel.py +++ b/clients/terminal/tui/widgets/chat_panel.py @@ -5,7 +5,7 @@ import json from typing import Any -from rich.console import Group, RenderableType +from rich.console import RenderableType from rich.text import Text from textual.app import ComposeResult from textual.containers import ScrollableContainer @@ -59,16 +59,57 @@ def _signature(msg: dict[str, Any]) -> str: """Stable serialized form of a renderer msg, for cache invalidation. - Only an item whose rendered inputs changed should miss the cache; this - captures content + meta so in-place mutations (e.g. a streaming assistant - bubble growing token by token, or a rolling planning_status line) invalidate - exactly their own entry. + Only an item whose rendered inputs changed should re-render; this captures + content + meta so in-place mutations (e.g. a streaming assistant bubble + growing token by token, or a rolling planning_status line) invalidate + exactly their own widget. """ return json.dumps(msg, sort_keys=True, default=str) +class _ChatItemView(Static): + """One chat item as its own widget. + + Making each message a distinct widget is what keeps streaming cheap: a + per-token ``stream_delta`` only re-renders *this* widget (via + ``Static.update``), not the whole conversation — Textual refreshes just the + changed region instead of re-laying-out every visible item. + """ + + DEFAULT_CSS = """ + _ChatItemView { + height: auto; + width: 1fr; + padding: 0; + } + """ + + def __init__(self, renderable: RenderableType, *, item: ChatItem, signature: str) -> None: + super().__init__(renderable) + self._item: ChatItem = item + self._signature: str = signature + + def maybe_update(self, item: ChatItem, registry) -> bool: + """Re-render only if the item's signature changed. Returns True if updated.""" + msg = _item_msg(item) + sig = _signature(msg) + if sig == self._signature: + return False + self._item = item + self._signature = sig + self.update(registry.render(msg)) + return True + + class ChatPanel(ScrollableContainer): - """Scrollable conversation panel.""" + """Scrollable conversation panel. + + Each visible message is a separate ``_ChatItemView`` child widget rather + than one big ``Static`` rebuilt on every event. On a per-token stream the + only widget that re-renders is the streaming bubble; everything else is + untouched. Items are mounted/unmounted incrementally as the model grows and + as the visible-window cap truncates the oldest. + """ DEFAULT_CSS = """ ChatPanel { @@ -88,40 +129,37 @@ super().__init__() self._model = ChatModel() self._registry = default_registry() - self._items_container = Static("") - # Renderable cache keyed by id(item) -> (signature, renderable). On a - # per-token stream_delta only the streaming item's signature changes, - # so every other visible item is reused from here instead of being - # rebuilt (and re-parsed as markdown) on every token. - self._chat_render_cache: dict[int, tuple[str, RenderableType]] = {} + # Truncation hint shown at the top when the history exceeds the cap. + self._hint_view = Static("", id="chat-truncation-hint") + self._hint_view.styles.display = "none" + # Number of older items currently truncated out of view (0 = none). + self._hidden_count = 0 + # item -> view widget, keyed by id(item). Only holds visible items, so + # it stays bounded by the window cap, not by total history length. + self._widgets: dict[int, _ChatItemView] = {} def compose(self) -> ComposeResult: - yield self._items_container - - def on_mount(self) -> None: - self._items_container.styles.height = "auto" + yield self._hint_view def add_user_message(self, text: str) -> None: self._model.add_user_message(text) - self._refresh() + self._sync() def load_history(self, messages: list[dict]) -> None: """Populate the chat panel with a session's stored messages on resume.""" self._model.load_history(messages) - self._chat_render_cache.clear() - self._refresh() + self._rebuild_all() def handle_ws_event(self, msg: dict) -> None: self._model.handle_ws_event(msg) - self._refresh() + self._sync() def clear(self) -> None: """Reset the chat model and redraw an empty conversation.""" self._model.items.clear() self._model._current_assistant = None self._model._current_thinking = None - self._chat_render_cache.clear() - self._refresh() + self._rebuild_all() def _max_visible_items(self) -> int: return get_tui_settings().max_visible_items @@ -133,35 +171,62 @@ style=theme.text.hex, ) - def _refresh(self) -> None: + def _rebuild_all(self) -> None: + """Unmount every item widget and re-sync from scratch. + + Used on session switch / clear, where the model is replaced wholesale: + the surviving-vs-new diff is meaningless when nothing survives. + """ + for widget in self._widgets.values(): + widget.remove() + self._widgets.clear() + self._sync() + + def _sync(self) -> None: + """Reconcile mounted widgets with the model's visible window. + + The model only ever appends new items and pops/purges existing ones — + it never reorders survivors — so new widgets always belong at the end + and a simple mount-at-end keeps the DOM in the right order. Surviving + items keep their widget and are only re-rendered if their signature + changed (in-place mutation, e.g. a growing stream). + """ items = self._model.items limit = self._max_visible_items() hidden = max(0, len(items) - limit) # Slice the last `limit` items when over the cap; otherwise keep all. visible = items[-limit:] if hidden else items + visible_ids = {id(it) for it in visible} - renderables: list[RenderableType] = [] + # Truncation hint at the top of the window. + self._hidden_count = hidden if hidden: - renderables.append(self._truncation_hint(hidden)) + self._hint_view.update(self._truncation_hint(hidden)) + self._hint_view.styles.display = "block" + else: + self._hint_view.styles.display = "none" - seen_ids: set[int] = set() + # Drop widgets whose item left the visible window (truncated out, or + # purged from the model by stream_end / plan_ready). + for key in [k for k in self._widgets if k not in visible_ids]: + widget = self._widgets.pop(key) + widget.remove() + + # Walk the visible window in order: mount new items (at the end) and + # re-render changed ones in place. for item in visible: - seen_ids.add(id(item)) - msg = _item_msg(item) - sig = _signature(msg) - cached = self._chat_render_cache.get(id(item)) - if cached is not None and cached[0] == sig: - renderables.append(cached[1]) + key = id(item) + widget = self._widgets.get(key) + if widget is None: + msg = _item_msg(item) + widget = _ChatItemView( + self._registry.render(msg), + item=item, + signature=_signature(msg), + ) + self._widgets[key] = widget + self.mount(widget) else: - rendered = self._registry.render(msg) - self._chat_render_cache[id(item)] = (sig, rendered) - renderables.append(rendered) + widget.maybe_update(item, self._registry) - # Drop cache entries for items no longer in the model so memory stays - # bounded by the live history, not by everything ever shown. - stale = [key for key in self._chat_render_cache if key not in seen_ids] - for key in stale: - self._chat_render_cache.pop(key, None) - - self._items_container.update(Group(*renderables)) self.scroll_end(animate=False) \ No newline at end of file diff --git a/tests/clients/test_chat_panel.py b/tests/clients/test_chat_panel.py index 2e98f63..5421228 100644 --- a/tests/clients/test_chat_panel.py +++ b/tests/clients/test_chat_panel.py @@ -1,8 +1,10 @@ -"""Tests for ChatPanel render caching and the visible-history window cap. +"""Tests for ChatPanel per-message widget rendering and the visible-window cap. -The panel rebuilds every renderable on each WS event (including per-token -stream_delta). Caching makes only the changed item miss, and the window cap -bounds how many items are laid out regardless of total history length. +Each message is its own ``_ChatItemView`` widget. On a per-token stream only the +streaming bubble's widget re-renders (signature-gated); everything else is +untouched. The window cap bounds how many item widgets stay mounted regardless +of total history length, and a truncation hint sits at the top when older +messages are dropped. """ from __future__ import annotations @@ -37,19 +39,6 @@ monkeypatch.setattr(api_module, "get_profile_model", lambda pid: "configured-model") -def _capture_groups(chat) -> list: - """Record each Group passed to the items container's update().""" - captured: list = [] - original_update = chat._items_container.update - - def _record(group, *args, **kwargs): - captured.append(group) - return original_update(group, *args, **kwargs) - - chat._items_container.update = _record # type: ignore[method-assign] - return captured - - def _wrap_render(chat) -> list: """Wrap the panel's renderer registry.render to count calls.""" calls: list[dict] = [] @@ -63,11 +52,16 @@ return calls +def _item_widgets(chat) -> list: + """The panel's mounted item widgets, in display (insertion) order.""" + return list(chat._widgets.values()) + + @pytest.mark.anyio async def test_per_token_refresh_renders_only_the_streaming_item() -> None: """A stream_delta changes only the assistant bubble's signature, so the - registry is called exactly once per token — every other visible item is - reused from the cache instead of being rebuilt/re-parsed.""" + registry is called exactly once per token — every other visible item + widget is left untouched instead of being rebuilt/re-parsed.""" async with NaviCodeTui(new_session=True).run_test() as pilot: await pilot.pause() chat = pilot.app.query_one("ChatPanel") @@ -78,7 +72,7 @@ chat.handle_ws_event({"type": "stream_delta", "delta": "a"}) # creates + renders assistant baseline = len(calls) - # Two more tokens — only the streaming assistant item should be rebuilt. + # Two more tokens — only the streaming assistant widget should re-render. chat.handle_ws_event({"type": "stream_delta", "delta": "b"}) after_one = len(calls) chat.handle_ws_event({"type": "stream_delta", "delta": "c"}) @@ -89,23 +83,58 @@ @pytest.mark.anyio -async def test_window_cap_renders_only_last_n_with_hint(monkeypatch) -> None: - """Beyond the cap, only the last N items are laid out and a hint is shown.""" +async def test_per_token_refresh_keeps_other_widgets_stable(monkeypatch) -> None: + """Re-rendering the streaming widget must NOT touch the other item widgets: + the same widget objects persist across tokens (no re-mount of older items).""" + async with NaviCodeTui(new_session=True).run_test() as pilot: + await pilot.pause() + chat = pilot.app.query_one("ChatPanel") + monkeypatch.setattr(chat, "_max_visible_items", lambda: 50) + chat.clear() + + chat.add_user_message("hi") + chat.handle_ws_event({"type": "stream_start"}) + chat.handle_ws_event({"type": "stream_delta", "delta": "a"}) + await pilot.pause() + + widgets_before = _item_widgets(chat) + user_widget = widgets_before[0] + assistant_widget = widgets_before[-1] + assert user_widget._item.kind == "user_message" + assert assistant_widget._item.kind == "assistant_message" + + # Two more tokens — the user widget must be the same object; only the + # assistant widget re-renders. + chat.handle_ws_event({"type": "stream_delta", "delta": "b"}) + chat.handle_ws_event({"type": "stream_delta", "delta": "c"}) + await pilot.pause() + + widgets_after = _item_widgets(chat) + assert widgets_after[0] is user_widget, "user bubble widget must not be re-mounted" + assert widgets_after[-1] is assistant_widget, "assistant widget updated in place, not re-mounted" + assert assistant_widget._item.content == "abc" + + +@pytest.mark.anyio +async def test_window_cap_mounts_only_last_n_with_hint(monkeypatch) -> None: + """Beyond the cap, only the last N item widgets stay mounted and a hint is shown.""" async with NaviCodeTui(new_session=True).run_test() as pilot: await pilot.pause() chat = pilot.app.query_one("ChatPanel") monkeypatch.setattr(chat, "_max_visible_items", lambda: 3) - captured = _capture_groups(chat) chat.clear() # drop the startup cwd/connected status items for i in range(5): chat.add_user_message(f"msg {i}") + await pilot.pause() - group = captured[-1] - # 3 visible items + 1 truncation hint. - assert len(group.renderables) == 4 - hint = group.renderables[0] - assert "2 earlier messages not shown" in str(hint) + widgets = _item_widgets(chat) + # Only the last 3 items are mounted. + assert len(widgets) == 3 + assert [w._item.content for w in widgets] == ["msg 2", "msg 3", "msg 4"] + # Truncation hint is visible and reports the 2 dropped oldest items. + assert chat._hidden_count == 2 + assert chat._hint_view.styles.display != "none" @pytest.mark.anyio @@ -114,48 +143,52 @@ await pilot.pause() chat = pilot.app.query_one("ChatPanel") monkeypatch.setattr(chat, "_max_visible_items", lambda: 10) - captured = _capture_groups(chat) chat.clear() # drop the startup cwd/connected status items for i in range(3): chat.add_user_message(f"msg {i}") + await pilot.pause() - group = captured[-1] - assert len(group.renderables) == 3 - assert "not shown" not in str(group.renderables[0]) + assert len(_item_widgets(chat)) == 3 + assert chat._hidden_count == 0 + assert chat._hint_view.styles.display == "none" @pytest.mark.anyio -async def test_clear_empties_chat_render_cache() -> None: +async def test_clear_unmounts_all_item_widgets() -> None: async with NaviCodeTui(new_session=True).run_test() as pilot: await pilot.pause() chat = pilot.app.query_one("ChatPanel") chat.add_user_message("hi") chat.handle_ws_event({"type": "stream_start"}) chat.handle_ws_event({"type": "stream_delta", "delta": "a"}) - assert chat._chat_render_cache # populated during refresh + await pilot.pause() + assert chat._widgets # item widgets are mounted chat.clear() - assert chat._chat_render_cache == {} + await pilot.pause() + assert chat._widgets == {} assert chat._model.items == [] @pytest.mark.anyio -async def test_removed_item_is_pruned_from_cache() -> None: - """stream_end purges empty assistant bubbles; their cache entries must go - too so the cache stays bounded by live items, not by everything shown.""" +async def test_removed_item_is_unmounted() -> None: + """stream_end purges empty assistant bubbles; their widget must be removed + so only live items stay mounted.""" async with NaviCodeTui(new_session=True).run_test() as pilot: await pilot.pause() chat = pilot.app.query_one("ChatPanel") chat.handle_ws_event({"type": "stream_start"}) chat.handle_ws_event({"type": "stream_delta", "delta": ""}) # empty bubble + await pilot.pause() empty_ids = {id(it) for it in chat._model.items if it.kind == "assistant_message"} assert empty_ids # the empty bubble exists before stream_end chat.handle_ws_event({"type": "stream_end", "content": ""}) - cached_ids = set(chat._chat_render_cache.keys()) - assert not (cached_ids & empty_ids) + await pilot.pause() + mounted_ids = set(chat._widgets.keys()) + assert not (mounted_ids & empty_ids) # And the empty bubble is no longer in the model. assert not any(it.kind == "assistant_message" for it in chat._model.items) @@ -288,7 +321,6 @@ async with NaviCodeTui(new_session=True).run_test() as pilot: await pilot.pause() chat = pilot.app.query_one("ChatPanel") - captured = _capture_groups(chat) chat.clear() chat.load_history( @@ -298,16 +330,21 @@ ] ) await pilot.pause() - group = captured[-1] - # Two rendered bubbles, no truncation hint. - assert len(group.renderables) == 2 + + widgets = _item_widgets(chat) + # Two item widgets mounted, no truncation hint. + assert len(widgets) == 2 + assert chat._hidden_count == 0 + # Items are mounted in order: user first, then assistant. + assert [w._item.kind for w in widgets] == ["user_message", "assistant_message"] + + # Each widget carries the rendered bubble for its item; re-rendering the + # item through the panel's own registry reproduces what's on screen. + from clients.terminal.tui.widgets.chat_panel import _item_msg console = Console(record=True, width=80, force_terminal=True, color_system=None) - - def _text(r) -> str: - console.print(r) - return console.export_text(clear=True) - - rendered = _text(group.renderables[0]) + _text(group.renderables[1]) + for widget in widgets: + console.print(chat._registry.render(_item_msg(widget._item))) + rendered = console.export_text(clear=True) assert "hi there" in rendered assert "hello back" in rendered