feat(renderer): group consecutive tool outputs under shared tree banners#183
Open
janfeddersen-wq wants to merge 4 commits intompfaffenberger:mainfrom
Open
feat(renderer): group consecutive tool outputs under shared tree banners#183janfeddersen-wq wants to merge 4 commits intompfaffenberger:mainfrom
janfeddersen-wq wants to merge 4 commits intompfaffenberger:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds grouping infrastructure to the rich renderer so consecutive messages of certain types render with a single banner and subsequent items as continuation (tree-style) lines. Tracks the last rendered message type, treats some messages as transparent, and updates many render methods and the sync/async dispatch to honor grouping. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
code_puppy/messaging/rich_renderer.py (2)
340-351:⚠️ Potential issue | 🟠 MajorAsync
renderpath does not update_last_rendered_type— grouping is silently broken.The sync path (
_render_sync, line 243) correctly updates_last_rendered_typeafter_do_render. However, the asyncrendermethod calls_do_renderdirectly at line 351 without updating grouping state. If the async path is ever used for rendering (e.g., viastart_asyncor a future async consumer), consecutive groupable messages will each print their own banner instead of grouping.Proposed fix
async def render(self, message: AnyMessage) -> None: """Render a message asynchronously (supports user input requests).""" # Handle async-only message types if isinstance(message, UserInputRequest): await self._render_user_input_request(message) elif isinstance(message, ConfirmationRequest): await self._render_confirmation_request(message) elif isinstance(message, SelectionRequest): await self._render_selection_request(message) else: # Use sync render for everything else - self._do_render(message) + self._render_sync(message)🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@code_puppy/messaging/rich_renderer.py` around lines 340 - 351, The async render method fails to update grouping state: after calling _do_render in render (async), set self._last_rendered_type to the rendered message's type just like _render_sync does so grouping works; locate render and, after the branch that calls self._do_render(message), assign the appropriate type to self._last_rendered_type (using the same logic/_type value used in _render_sync) so async-rendered messages update the grouping state.
243-253:⚠️ Potential issue | 🟡 MinorRendering error leaves stale grouping state — next message may incorrectly skip its banner.
If
_do_renderraises (caught at line 251),_last_rendered_typeretains the value from the previous successful render. If the next message happens to be the same groupable type as that stale value,_is_continuationreturnsTrueand the banner is suppressed — even though no banner was actually printed in the failed render. Consider resetting_last_rendered_typetoNonein the except block.Proposed fix
except Exception as e: safe_error = escape_rich_markup(str(e)) self._console.print(f"[dim red]Render error: {safe_error}[/dim red]") + self._last_rendered_type = None # Reset grouping on error🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@code_puppy/messaging/rich_renderer.py` around lines 243 - 253, The except block in _render_sync leaves _last_rendered_type unchanged after a failed _do_render, causing subsequent messages of the same groupable type to be treated as continuations and skip their banner; modify the except block in _render_sync so that after catching the exception you reset self._last_rendered_type = None (or otherwise clear grouping state), then log the escaped error as before; ensure you still respect _TRANSPARENT_TYPES logic elsewhere and only clear the state on failures inside _render_sync.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@code_puppy/messaging/rich_renderer.py`:
- Around line 114-118: The class-level mutable sets _GROUPABLE_TYPES and
_TRANSPARENT_TYPES should be made immutable: replace their final populated
values with frozenset instances instead of mutating set() after import-time
population; update the comment to remove the misleading "Populated after message
imports are available" note (or move the definitions to module-level constants)
and ensure the population code that currently builds these sets (the
module-level population block) constructs frozenset(...) and assigns it to the
class attributes (or assigns module-level FROZEN_GROUPABLE_TYPES /
FROZEN_TRANSPARENT_TYPES) so callers see immutable containers.
- Around line 1164-1172: The class attributes
RichConsoleRenderer._GROUPABLE_TYPES and _TRANSPARENT_TYPES are being mutated
after class definition; change them to stable declarations instead: define
module-level frozenset constants (e.g., GROUPABLE_TYPES =
frozenset({FileContentMessage, GrepResultMessage, DiffMessage,
FileListingMessage, ShellStartMessage}) and TRANSPARENT_TYPES =
frozenset({SpinnerControl})) or set them directly inside the RichConsoleRenderer
class body (_GROUPABLE_TYPES = GROUPABLE_TYPES, _TRANSPARENT_TYPES =
TRANSPARENT_TYPES) and remove the post-definition mutation block that references
FileContentMessage, GrepResultMessage, DiffMessage, FileListingMessage,
ShellStartMessage, and SpinnerControl so the sets are populated
deterministically at import time.
- Around line 340-351: The async render method fails to update grouping state:
after calling _do_render in render (async), set self._last_rendered_type to the
rendered message's type just like _render_sync does so grouping works; locate
render and, after the branch that calls self._do_render(message), assign the
appropriate type to self._last_rendered_type (using the same logic/_type value
used in _render_sync) so async-rendered messages update the grouping state.
- Around line 243-253: The except block in _render_sync leaves
_last_rendered_type unchanged after a failed _do_render, causing subsequent
messages of the same groupable type to be treated as continuations and skip
their banner; modify the except block in _render_sync so that after catching the
exception you reset self._last_rendered_type = None (or otherwise clear grouping
state), then log the escaped error as before; ensure you still respect
_TRANSPARENT_TYPES logic elsewhere and only clear the state on failures inside
_render_sync.
In `@tests/test_rich_renderer.py`:
- Around line 675-707: Add an async test that mirrors
test_grouping_resets_across_different_groupable_types but exercises the async
render path: create renderer and console via _make_renderer(), mock
renderer._get_banner_color, construct a FileContentMessage and a
GrepResultMessage, await renderer.render(file_msg) then await
renderer.render(grep_msg), collect printed strings from
console.print.call_args_list, and assert that both "READ FILE" and "GREP"
banners are present; this will exercise render -> _do_render and ensure
_last_rendered_type is handled in the async path.
🧹 Nitpick comments (3)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@code_puppy/messaging/rich_renderer.py`: - Around line 114-118: The class-level mutable sets _GROUPABLE_TYPES and _TRANSPARENT_TYPES should be made immutable: replace their final populated values with frozenset instances instead of mutating set() after import-time population; update the comment to remove the misleading "Populated after message imports are available" note (or move the definitions to module-level constants) and ensure the population code that currently builds these sets (the module-level population block) constructs frozenset(...) and assigns it to the class attributes (or assigns module-level FROZEN_GROUPABLE_TYPES / FROZEN_TRANSPARENT_TYPES) so callers see immutable containers. - Around line 1164-1172: The class attributes RichConsoleRenderer._GROUPABLE_TYPES and _TRANSPARENT_TYPES are being mutated after class definition; change them to stable declarations instead: define module-level frozenset constants (e.g., GROUPABLE_TYPES = frozenset({FileContentMessage, GrepResultMessage, DiffMessage, FileListingMessage, ShellStartMessage}) and TRANSPARENT_TYPES = frozenset({SpinnerControl})) or set them directly inside the RichConsoleRenderer class body (_GROUPABLE_TYPES = GROUPABLE_TYPES, _TRANSPARENT_TYPES = TRANSPARENT_TYPES) and remove the post-definition mutation block that references FileContentMessage, GrepResultMessage, DiffMessage, FileListingMessage, ShellStartMessage, and SpinnerControl so the sets are populated deterministically at import time. In `@tests/test_rich_renderer.py`: - Around line 675-707: Add an async test that mirrors test_grouping_resets_across_different_groupable_types but exercises the async render path: create renderer and console via _make_renderer(), mock renderer._get_banner_color, construct a FileContentMessage and a GrepResultMessage, await renderer.render(file_msg) then await renderer.render(grep_msg), collect printed strings from console.print.call_args_list, and assert that both "READ FILE" and "GREP" banners are present; this will exercise render -> _do_render and ensure _last_rendered_type is handled in the async path.tests/test_rich_renderer.py (1)
675-707: Consider adding a test for asyncrendergrouping path.The async
rendermethod (line 340-351 inrich_renderer.py) calls_do_renderwithout updating_last_rendered_type. A test exercising grouping through the async path would catch this gap. Relates to the issue flagged in the renderer file.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@tests/test_rich_renderer.py` around lines 675 - 707, Add an async test that mirrors test_grouping_resets_across_different_groupable_types but exercises the async render path: create renderer and console via _make_renderer(), mock renderer._get_banner_color, construct a FileContentMessage and a GrepResultMessage, await renderer.render(file_msg) then await renderer.render(grep_msg), collect printed strings from console.print.call_args_list, and assert that both "READ FILE" and "GREP" banners are present; this will exercise render -> _do_render and ensure _last_rendered_type is handled in the async path.code_puppy/messaging/rich_renderer.py (2)
114-118: Class-level mutable sets could befrozensetfor clarity.Since
_GROUPABLE_TYPESand_TRANSPARENT_TYPESare populated once at module level (lines 1164-1172) and never mutated after that, usingfrozensetat the population site would communicate immutability intent. Also, the comment "Populated after message imports are available" is misleading — the message classes are already imported at lines 31-56 (top of file), so the sets could be defined inline in the class body or as module-level constants.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@code_puppy/messaging/rich_renderer.py` around lines 114 - 118, The class-level mutable sets _GROUPABLE_TYPES and _TRANSPARENT_TYPES should be made immutable: replace their final populated values with frozenset instances instead of mutating set() after import-time population; update the comment to remove the misleading "Populated after message imports are available" note (or move the definitions to module-level constants) and ensure the population code that currently builds these sets (the module-level population block) constructs frozenset(...) and assigns it to the class attributes (or assigns module-level FROZEN_GROUPABLE_TYPES / FROZEN_TRANSPARENT_TYPES) so callers see immutable containers.
1164-1172: Consider defining these as module-level constants instead of mutating class attributes post-definition.This pattern of initializing empty class-level sets and populating them after the class body works, but it's fragile — a subclass or test could inadvertently observe the empty sets if it references the class before this module-level code executes. Since all the message classes are already imported at the top of the file, you could define them inline in the class body or as module-level
frozensetconstants.Example alternative
class RichConsoleRenderer: - _GROUPABLE_TYPES: set = set() - _TRANSPARENT_TYPES: set = set() + _GROUPABLE_TYPES: frozenset = frozenset({ + FileContentMessage, + GrepResultMessage, + DiffMessage, + FileListingMessage, + ShellStartMessage, + }) + _TRANSPARENT_TYPES: frozenset = frozenset({SpinnerControl})And remove lines 1164-1172.
🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@code_puppy/messaging/rich_renderer.py` around lines 1164 - 1172, The class attributes RichConsoleRenderer._GROUPABLE_TYPES and _TRANSPARENT_TYPES are being mutated after class definition; change them to stable declarations instead: define module-level frozenset constants (e.g., GROUPABLE_TYPES = frozenset({FileContentMessage, GrepResultMessage, DiffMessage, FileListingMessage, ShellStartMessage}) and TRANSPARENT_TYPES = frozenset({SpinnerControl})) or set them directly inside the RichConsoleRenderer class body (_GROUPABLE_TYPES = GROUPABLE_TYPES, _TRANSPARENT_TYPES = TRANSPARENT_TYPES) and remove the post-definition mutation block that references FileContentMessage, GrepResultMessage, DiffMessage, FileListingMessage, ShellStartMessage, and SpinnerControl so the sets are populated deterministically at import time.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/test_rich_renderer.py (2)
449-672: Consider extracting the repeatedprintedfilter into a test helper.The pattern of collecting printed strings from mock calls is duplicated across all seven new tests. A small helper would reduce noise and make future tests easier to write.
Example helper
def _get_printed_strings(console: Mock) -> list[str]: """Extract all plain string arguments from console.print calls.""" return [ call.args[0] for call in console.print.call_args_list if call.args and isinstance(call.args[0], str) ]Then usage becomes:
printed = _get_printed_strings(console)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_rich_renderer.py` around lines 449 - 672, The tests repeat the same logic to extract printed strings from console.print mocks; add a small helper function named _get_printed_strings(console: Mock) that returns the list of plain string args from console.print.call_args_list, then replace the duplicated list comprehensions in the test functions (e.g., test_consecutive_file_content_groups_under_single_banner, test_single_file_content_still_has_banner_and_tree, test_different_type_breaks_grouping, test_spinner_does_not_break_grouping, test_consecutive_diff_groups, test_consecutive_shell_start_groups) with a single call to _get_printed_strings(console) to reduce duplication and improve readability.
449-742: Consider adding a test for grouping state reset on render error.The implementation resets
_last_rendered_type = Nonein theexceptblock of_render_sync(rich_renderer.py Line 263). There's no test verifying that an error during rendering of a groupable message correctly breaks the group for the next message of the same type.Example test
def test_render_error_resets_grouping() -> None: """A render error should break grouping for subsequent messages.""" renderer, console = _make_renderer() renderer._get_banner_color = Mock(return_value="blue") msg1 = FileContentMessage( path="/src/a.py", content="a", start_line=None, num_lines=None, total_lines=1, num_tokens=1, ) # First renders normally renderer._render_sync(msg1) # Inject a failure for the next render original = renderer._do_render def boom(m): if getattr(m, 'path', '') == '/src/err.py': raise RuntimeError("boom") original(m) renderer._do_render = boom err_msg = FileContentMessage( path="/src/err.py", content="x", start_line=None, num_lines=None, total_lines=1, num_tokens=1, ) renderer._render_sync(err_msg) # Restore and render another — should get a new banner renderer._do_render = original msg3 = FileContentMessage( path="/src/b.py", content="b", start_line=None, num_lines=None, total_lines=1, num_tokens=1, ) renderer._render_sync(msg3) printed = [ call.args[0] for call in console.print.call_args_list if call.args and isinstance(call.args[0], str) ] banner_lines = [p for p in printed if "READ FILE" in p] assert len(banner_lines) == 2, f"Expected 2 banners (error should reset group): {banner_lines}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_rich_renderer.py` around lines 449 - 742, Add a new test ensuring that when _render_sync raises an exception the grouping state is reset by verifying _last_rendered_type becomes None for the next groupable message; specifically, follow the pattern in the suggested example: render a FileContentMessage via renderer._render_sync, monkeypatch renderer._do_render to raise for a second FileContentMessage (path '/src/err.py') so _render_sync hits the except block that sets _last_rendered_type = None, then restore _do_render and render a third FileContentMessage and assert there are two "READ FILE" banners printed; reference _render_sync, _do_render, and _last_rendered_type to locate the behavior in rich_renderer.py.code_puppy/messaging/rich_renderer.py (2)
419-425: Trailing\nin tree-child line creates visual inconsistency with other grouped renderers.Line 424 includes a trailing
\ninside the f-string, which adds an extra blank line after the tree-child entry. The other grouped renderers (_render_file_contentat Line 550,_render_grep_resultat Line 565,_render_diffat Line 655,_render_shell_startat Line 704) do not include this trailing newline. When multipleFileListingMessageitems are grouped, this results in extra vertical spacing between entries that doesn't appear for other grouped types.Remove trailing `\n` for consistency
self._console.print( - f" ├─ 📂 [bold cyan]{msg.directory}[/bold cyan] [dim]{rec_flag}[/dim]\n" + f" ├─ 📂 [bold cyan]{msg.directory}[/bold cyan] [dim]{rec_flag}[/dim]" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code_puppy/messaging/rich_renderer.py` around lines 419 - 425, The tree-child print in _render for FileListingMessage is appending a trailing "\n" in the f-string which causes extra blank lines when grouped; locate the block that checks self._is_continuation(FileListingMessage) and prints via self._console.print(f" ├─ 📂 [bold cyan]{msg.directory}[/bold cyan] [dim]{rec_flag}[/dim]\n") and remove the trailing "\n" so the call becomes consistent with other grouped renderers (keep the rest of the string and surrounding logic unchanged).
125-129: Consider exportingGROUPABLE_TYPESandTRANSPARENT_TYPESin__all__.The module-level
GROUPABLE_TYPESandTRANSPARENT_TYPES(Lines 101–110) are public-facing constants (no underscore prefix), but they're not listed in__all__. If they're meant to be internal, the convention would be to prefix them with_. If external consumers (plugins, tests) should reference them, add them to__all__.Also applies to: 1180-1184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code_puppy/messaging/rich_renderer.py` around lines 125 - 129, The public constants GROUPABLE_TYPES and TRANSPARENT_TYPES are not exported via __all__ while their underscored aliases _GROUPABLE_TYPES and _TRANSPARENT_TYPES are used internally; decide whether they are public or private and make it consistent: if they should be public, add "GROUPABLE_TYPES" and "TRANSPARENT_TYPES" to the module's __all__ list; if they should be private, rename the top-level symbols to _GROUPABLE_TYPES and _TRANSPARENT_TYPES (or remove the non-underscored names) and update any references to use the underscored names (e.g., where _GROUPABLE_TYPES and _TRANSPARENT_TYPES are assigned) so the module export surface is correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code_puppy/messaging/rich_renderer.py`:
- Around line 419-425: The tree-child print in _render for FileListingMessage is
appending a trailing "\n" in the f-string which causes extra blank lines when
grouped; locate the block that checks self._is_continuation(FileListingMessage)
and prints via self._console.print(f" ├─ 📂 [bold cyan]{msg.directory}[/bold
cyan] [dim]{rec_flag}[/dim]\n") and remove the trailing "\n" so the call becomes
consistent with other grouped renderers (keep the rest of the string and
surrounding logic unchanged).
- Around line 125-129: The public constants GROUPABLE_TYPES and
TRANSPARENT_TYPES are not exported via __all__ while their underscored aliases
_GROUPABLE_TYPES and _TRANSPARENT_TYPES are used internally; decide whether they
are public or private and make it consistent: if they should be public, add
"GROUPABLE_TYPES" and "TRANSPARENT_TYPES" to the module's __all__ list; if they
should be private, rename the top-level symbols to _GROUPABLE_TYPES and
_TRANSPARENT_TYPES (or remove the non-underscored names) and update any
references to use the underscored names (e.g., where _GROUPABLE_TYPES and
_TRANSPARENT_TYPES are assigned) so the module export surface is correct.
In `@tests/test_rich_renderer.py`:
- Around line 449-672: The tests repeat the same logic to extract printed
strings from console.print mocks; add a small helper function named
_get_printed_strings(console: Mock) that returns the list of plain string args
from console.print.call_args_list, then replace the duplicated list
comprehensions in the test functions (e.g.,
test_consecutive_file_content_groups_under_single_banner,
test_single_file_content_still_has_banner_and_tree,
test_different_type_breaks_grouping, test_spinner_does_not_break_grouping,
test_consecutive_diff_groups, test_consecutive_shell_start_groups) with a single
call to _get_printed_strings(console) to reduce duplication and improve
readability.
- Around line 449-742: Add a new test ensuring that when _render_sync raises an
exception the grouping state is reset by verifying _last_rendered_type becomes
None for the next groupable message; specifically, follow the pattern in the
suggested example: render a FileContentMessage via renderer._render_sync,
monkeypatch renderer._do_render to raise for a second FileContentMessage (path
'/src/err.py') so _render_sync hits the except block that sets
_last_rendered_type = None, then restore _do_render and render a third
FileContentMessage and assert there are two "READ FILE" banners printed;
reference _render_sync, _do_render, and _last_rendered_type to locate the
behavior in rich_renderer.py.
Owner
|
@nhicks00 - Wanna try this? |
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
2 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.
Problem
Tool output banners were repeated for every event, which made the terminal noisy and hard to scan during rapid tool usage (especially repeated READ FILE/GREP calls).
After initial grouping, GREP sub-results still visually broke the tree structure.
What changed
Implemented immediate (non-buffered) grouping for consecutive tool outputs in RichConsoleRenderer:
• Added renderer grouping state (_last_rendered_type)
• Added group handling helpers:
◦ _GROUPABLE_TYPES
◦ _TRANSPARENT_TYPES
◦ _is_continuation(...)
• Updated grouped rendering for:
◦ READ FILE
◦ GREP
◦ EDIT FILE
◦ DIRECTORY LISTING
◦ SHELL COMMAND
GREP tree fix
Improved GREP formatting so sub-lines remain visually connected to their parent query entry:
• prefixed sub-lines with │ connectors
• removed stray blank line in concise mode
• removed trailing newline that broke consecutive tree flow
Behavior example
Before:
READ FILE /file1
READ FILE /file2
READ FILE /file3
After:
READ FILE
├─ 📂 /file1
├─ 📂 /file2
├─ 📂 /file3
Testing
Expanded renderer tests to verify:
• grouped banner behavior for consecutive same-type messages
• single-item banner+tree rendering
• grouping reset on different message types
• spinner transparency (does not break groups)
• grouped behavior for diff/shell
• cross-type grouping transitions
All targeted suites pass (tests/test_rich_renderer.py and broader messaging/rendering regression run).
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests