feat: add tool call response folding#154
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements native Neovim folding for tool-call response bodies in the chat UI. It adds a new agentic.ui.ChatFolds module to track extmark-backed tool-call blocks, compute effective folding policy from config, create body-only manual folds with custom foldtext, preserve and restore per-window user fold state, and defer/backfill folds when the chat buffer has no visible window. MessageWriter and SessionManager are wired into ChatFolds; ChatWidget gains a before-hide hook to capture state. It also adds config defaults, docs, tests, benchmarks, and a Makefile benchmark target. Sequence Diagram(s)sequenceDiagram
participant User
participant MessageWriter as MessageWriter
participant ChatFolds as ChatFolds
participant Neovim as Neovim (extmarks/folds)
participant Window as Window
User->>MessageWriter: write_tool_call_block(tracker)
MessageWriter->>MessageWriter: set fold_text_prefix
MessageWriter->>Neovim: place extmarks for header/body/footer
MessageWriter->>ChatFolds: sync_tool_call(tracker)
ChatFolds->>ChatFolds: resolve policy (enabled, kind, min_lines, default_closed)
alt Visible window(s) present
ChatFolds->>Window: set foldmethod=manual, foldenable=true
ChatFolds->>Neovim: compute body range, create fold, set foldtext
ChatFolds->>Neovim: foldclose / foldopen as needed
else No visible window
ChatFolds->>ChatFolds: record pending for BufWinEnter backfill
end
sequenceDiagram
participant ChatWidget as ChatWidget
participant SessionManager as SessionManager
participant ChatFolds as ChatFolds
participant Buffer as Buffer
ChatWidget->>SessionManager: hide()
SessionManager->>ChatFolds: remember_visible_window_states()
ChatFolds->>Buffer: capture per-window fold open/closed states
ChatWidget->>ChatWidget: close widget
ChatWidget->>SessionManager: show()
SessionManager->>ChatFolds: on_buf_win_enter(chat_winid)
ChatFolds->>ChatFolds: sync_pending() / sync_reopen_states()
ChatFolds->>Buffer: recreate/restore folds with preserved user state
Possibly related PRs
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 |
|
Hey @jfahrer, thanks for this initial implementation, going with the native Neovim folding seems right to me. My initial thought was to "trim" the content and rewrite when the user pressed a keybind, but using native features is always preferred to reduce the plugin scope. I'll do an in-depth review throughout the week to make sure we have the right pieces in the right places. Even tho you said Codex and Claude already reviewed, they struggle to follow patterns and sometimes duplicate code or write in the wrong file. In the meantime, it would be amazing if you could record a short video demonstrating how yopur solution is working for you and you pressing SIDE NOTE: it seems OpenCode changed their message format 😞 |
carlos-algms
left a comment
carlos-algms
left a comment
There was a problem hiding this comment.
I would say it's a good start.
I got this from the docs:
From the docs (line 42-43):
> "The manual folds are lost when you abandon the file."
And from fold-behavior (line 644-648):
> "When editing a buffer that has been edited before, the last used folding settings are used again. For manual folding the defined folds are restored. [...] If this buffer has been edited in this window, the values from back then are used. Otherwise the values from the window where the buffer was edited last are used."
So Neovim does **try to restore** manual folds when re-displaying a buffer, but this is best-effort based on internal buffer edit history. It's not a guarantee — if the buffer was never edited in any window, or the window state was fully cleaned up, folds are gone.
So we might be able to get away with setting and forgetting the fold only once per tool call, and not need to update all tool call blocks all the time.
The current implementation is naive and doesn't take performance into consideration.
Try another round, ask your AI to read Neovim docs, steer it in the right direction and let's see.
| if type(tool_calls.enabled) == "boolean" then | ||
| enabled = tool_calls.enabled | ||
| end | ||
|
|
||
| if type(kind_policy.enabled) == "boolean" then | ||
| enabled = kind_policy.enabled | ||
| end |
There was a problem hiding this comment.
You'll probably want to invert this and turn into an elseif
kind has higher priority and ONLY check for tool_calls if kind is nil.
| if type(tool_calls.min_lines) == "number" then | ||
| min_lines = tool_calls.min_lines | ||
| end | ||
| if type(kind_policy.min_lines) == "number" then | ||
| min_lines = kind_policy.min_lines | ||
| end |
There was a problem hiding this comment.
Same, if both are set, one overrides the other.
|
Thanks for the feedback! I appreciate you taking a look so swiftly. I should have some time on Friday to take a closer look. |
|
I was interested in this too! One consideration: folding is pretty nice for this, but at the same time, folding can be a bit tricky because it will interact with a user's fold configuration and that may make this not work "as intended" based on how users configure neovim, which may result in them having to edit their folds/autocmds/etc specifically for agentic. I believe that virtual text is already being used in the places where the contents of a tool call response is used, so it might be better to hide the content using virtual text instead. It's also more flexible (nvim-ufo uses virtual text for it's folds for this reason). |
jfahrer
force-pushed
the
main
branch
2 times, most recently
from
9aedd07 to
5bd4cfa
Compare
jfahrer
force-pushed
the
main
branch
2 times, most recently
from
8d4c567 to
f78400a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/agentic/ui/chat_folds.lua`:
- Around line 176-185: The code currently forces window fold settings
unconditionally in ChatFolds:_configure_window which alters user/autocmd
behavior even when folding.tool_calls.enabled is false; update the logic so
_configure_window only applies the fold options when folding is enabled (check
the feature flag folding.tool_calls.enabled or an equivalent ChatFolds instance
flag) or only call _configure_window from places that create folds (e.g.,
sync_tool_call) when folds are actually created; specifically, add a guard
around the body of ChatFolds:_configure_window to return early if folding is
disabled, or remove the unconditional calls to ChatFolds:_configure_window and
invoke it conditionally where folds are created so
foldmethod/foldenable/foldtext are not set when the feature is off.
- Around line 24-28: The ChatFolds implementation currently uses win_findbuf()
which walks every tabpage and thus applies fold state globally; change ChatFolds
to store an owning tabpage id (e.g., tab_page_id on the ChatFolds instance
created in ChatFolds:new) and replace any use of win_findbuf()/global window
enumeration with tabpage-scoped APIs (use
vim.api.nvim_tabpage_list_wins(tab_page_id) or equivalent) so only windows in
that tab are considered when capturing/applying folds (references: ChatFolds,
ChatFolds:new, and any methods around the previous win_findbuf usage such as the
fold capture/apply routines); also update all ChatFolds:new(...) call sites to
pass the correct owning tabpage id.
- Around line 410-423: The current check in ChatFolds:sync_reopen_states only
resyncs when _get_fold_state(winid, body_start_row + 1) returns nil, which
misses cases where a fold exists but its end is shorter than the extmark body
range; change the logic in ChatFolds:sync_reopen_states to call
_resolve_body_range(tool_call_fold.extmark_id) to get body_start_row and
body_line_count and then compare the existing window fold's end/length (via
_get_fold_state or by querying the fold bounds returned by it) against
body_line_count — if there is no fold or the existing fold does not extend to
body_start_row + body_line_count, call _sync_fold_to_window(winid,
tool_call_fold) to fully resync the extmark range for tool_call_id from
_tool_call_folds while iterating _reopen_restore_tool_call_ids.
In `@lua/agentic/ui/chat_widget.lua`:
- Around line 155-157: hide() currently invokes self._on_before_hide at the top
and can be re-entered, causing the callback to run multiple times and overwrite
snapshots; add a re-entrancy guard (e.g., self._is_hiding) in hide(): if
self._is_hiding then return early, otherwise set self._is_hiding = true before
calling self._on_before_hide (and before other teardown), and clear
self._is_hiding = false at the end of hide() (ensure it is cleared in all exit
paths) so _on_before_hide runs only once per logical hide.
In `@Makefile`:
- Line 10: The Makefile is missing required .PHONY targets "all" and "clean"
which checkmake flags; add minimal phony entries and corresponding stub targets
to satisfy the linter: update the existing .PHONY line (which currently lists
luals selene ...) to also include all and clean, and add simple rules for "all"
(e.g., default no-op or alias to your primary build/validate target like "check"
or "format") and "clean" (no-op or remove generated artifacts) so Makefile's
phony targets include all and clean and the Makefile defines those targets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3a3ffcf9-0dfe-4c0f-9355-671f37630966
📒 Files selected for processing (18)
MakefileREADME.mdlua/agentic/config_default.lualua/agentic/session_manager.lualua/agentic/session_manager.test.lualua/agentic/ui/chat_folds.lualua/agentic/ui/chat_folds.test.lualua/agentic/ui/chat_widget.lualua/agentic/ui/chat_widget.test.lualua/agentic/ui/file_picker.lualua/agentic/ui/message_writer.lualua/agentic/utils/extmark_block.luaopenspec/changes/add-tool-call-response-folding/design.mdopenspec/changes/add-tool-call-response-folding/proposal.mdopenspec/changes/add-tool-call-response-folding/specs/tool-call-folding/spec.mdopenspec/changes/add-tool-call-response-folding/tasks.mdselene.tomltests/benchmarks/tool_call_response_folding.lua
There was a problem hiding this comment.
Hey @carlos-algms! Sorry for the long turn around. I got caught up at work.
I haven't had time to do a full pass of the code and some of your previous comments still apply. I hope I can do another pass later this week. I'll go over the Coderabbit comments as well. The PR ended up getting fairly large. Let me know if you want to split this up.
- Added benchmark coverage for tool call folding (run with
make benchmark-tool-call-folding)- Average results:
- hot-path-sync-one-updated-call: 17.37 ms total, 0.0868 ms/op in a 1574 line transcript
- hidden-bufwinenter-backfill: 0.45 ms for 6 pending folds restored on reopen
- reopen-with-restored-folds: 0.005 ms for 40 existing folds and 0 pending
- initial-visible-many-completed: 22.50 ms total, 0.1875 ms/op in a 2043 line transcript
- Average results:
- Moved all the folding logic into its own module. Session manager sets up callbacks to glue everything together.
- Fold restoration now relies on neovim's native behavior
| @@ -0,0 +1,366 @@ | |||
| local ChatFolds = require("agentic.ui.chat_folds") | |||
There was a problem hiding this comment.
I added a benchmarking test for the chat folding. This was primarily to ensure the performance is good. I don't think we should merge this, but wanted to get your take.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/agentic/session_manager.lua`:
- Around line 331-339: The BufWinEnter callback in
SessionManager:_bind_chat_buffer_events currently uses
vim.api.nvim_get_current_win(), which can be wrong during rapid switches;
replace that with vim.fn.bufwinid(self.widget.buf_nrs.chat) to get the window ID
that actually displays the chat buffer, and only call
self.chat_folds:on_buf_win_enter(winid) when bufwinid returns a valid winid (>=
0) so you avoid passing an invalid window to chat_folds:on_buf_win_enter.
In `@tests/benchmarks/tool_call_response_folding.lua`:
- Around line 344-364: M.run currently calls benchmark_hot_path_sync,
benchmark_hidden_backfill, benchmark_reopen_restored, and
benchmark_initial_visible_creation once each and prints single-run results;
update these benchmarks to accept configurable iteration counts and add optional
warmup iterations and multiple measurement runs, then modify M.run to pass an
iterations/warmup parameter, execute each benchmark multiple times, compute
min/max/avg (and optionally stddev) across runs, and change the calls to
print_result to output aggregated statistics rather than single-run values so
results are statistically meaningful (refer to functions
benchmark_hot_path_sync, benchmark_hidden_backfill, benchmark_reopen_restored,
benchmark_initial_visible_creation, M.run, and print_result).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3961b055-0f26-4303-92c4-ed027679c21b
📒 Files selected for processing (7)
Makefilelua/agentic/session_manager.lualua/agentic/ui/chat_folds.lualua/agentic/ui/chat_folds.test.lualua/agentic/ui/chat_widget.lualua/agentic/ui/chat_widget.test.luatests/benchmarks/tool_call_response_folding.lua
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lua/agentic/ui/file_picker.lua (1)
229-235:⚠️ Potential issue | 🟠 MajorExclude
.worktrees/in the primary scan path too.This new pattern only runs in the glob fallback, but
scan_files()normally returns fromrg/fd/git ls-filesfirst. On most setups that means.worktrees/entries will still show up in completion despite this change.🔧 Suggested fix
- for line in output:gmatch("[^\n]+") do - if line ~= "" then - local relative_path = FileSystem.to_smart_path(line) - table.insert(files, { - word = "@" .. relative_path, - menu = "File", - kind = "@", - icase = 1, - }) - end - end + for line in output:gmatch("[^\n]+") do + if line ~= "" then + local relative_path = FileSystem.to_smart_path(line) + if not self:_should_exclude(relative_path) then + table.insert(files, { + word = "@" .. relative_path, + menu = "File", + kind = "@", + icase = 1, + }) + end + end + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/agentic/ui/file_picker.lua` around lines 229 - 235, The .worktrees/ pattern was only added to FilePicker.GLOB_EXCLUDE_PATTERNS (used by the glob fallback) so entries still appear when scan_files() uses primary tools (rg/fd/git); update the primary scan exclusion set used by scan_files() to also exclude "^%.worktrees/" (or add the same pattern to whatever variable or args scan_files() passes to rg/fd/git), e.g., extend the primary exclude list or add the "^%.worktrees/" pattern to the options passed by scan_files() so .worktrees/ is filtered out before falling back to the glob.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/agentic/config_default.lua`:
- Around line 454-475: The new configuration table agentic.UserConfig.Folding
(specifically folding.tool_calls and its keys: enabled, closed_by_default,
min_lines, and kinds -> fetch/execute/edit) is not documented; add a vimdoc
section describing each option and defaults, format text to 78‑char line width
with right‑aligned tags for each option, place the section into the plugin help
file used for agentic docs, and finally regenerate help tags by running a
headless Neovim helptags command (e.g., timeout + headless nvim -c 'helptags
<docdir>' -c 'quit').
In `@lua/agentic/ui/chat_widget.lua`:
- Around line 101-103: Duplicate "window became visible" signaling is introduced
by directly calling self._on_after_show(self.win_nrs.chat) here; remove that
call and rely on the existing chat-buffer BufWinEnter route
(chat_folds.on_buf_win_enter) as the single visibility hook/source-of-truth so
pending-fold consumption and fold restoration ordering remain consistent; ensure
no other code paths still invoke _on_after_show for the same open to avoid
duplicate signals.
In `@lua/agentic/ui/chat_widget.test.lua`:
- Around line 108-116: The test currently asserts before_hide_spy.call_count > 0
which allows multiple invocations; change the assertion to assert that the spy
was called exactly once to lock the contract. Locate the test which sets
before_hide_spy via widget:set_on_before_hide, calls widget:show() and
widget:hide(), and replace the final assertion with a strict equality check (use
before_hide_spy.call_count == 1 or the test framework's exact-call matcher) so
the test fails if hide() invokes the callback more than once.
In `@README.md`:
- Around line 689-694: Reword the first sentence under the "Tool Call Folding"
heading to state that both completed and failed tool call responses are foldable
(failed responses are created with a native fold but start open), so the intro
no longer implies only completed responses use folds; locate the "Tool Call
Folding" section and update the opening line to clearly mention completed,
in-progress, and failed tool calls and their default fold states.
---
Outside diff comments:
In `@lua/agentic/ui/file_picker.lua`:
- Around line 229-235: The .worktrees/ pattern was only added to
FilePicker.GLOB_EXCLUDE_PATTERNS (used by the glob fallback) so entries still
appear when scan_files() uses primary tools (rg/fd/git); update the primary scan
exclusion set used by scan_files() to also exclude "^%.worktrees/" (or add the
same pattern to whatever variable or args scan_files() passes to rg/fd/git),
e.g., extend the primary exclude list or add the "^%.worktrees/" pattern to the
options passed by scan_files() so .worktrees/ is filtered out before falling
back to the glob.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1b2242e-9a69-4551-a630-75678f8fa9b5
📒 Files selected for processing (17)
MakefileREADME.mdlua/agentic/config_default.lualua/agentic/session_manager.lualua/agentic/session_manager.test.lualua/agentic/ui/chat_folds.lualua/agentic/ui/chat_folds.test.lualua/agentic/ui/chat_widget.lualua/agentic/ui/chat_widget.test.lualua/agentic/ui/file_picker.lualua/agentic/ui/message_writer.lualua/agentic/utils/extmark_block.luaopenspec/changes/add-tool-call-response-folding/design.mdopenspec/changes/add-tool-call-response-folding/proposal.mdopenspec/changes/add-tool-call-response-folding/specs/tool-call-folding/spec.mdopenspec/changes/add-tool-call-response-folding/tasks.mdtests/benchmarks/tool_call_response_folding.lua
| it("hide() calls the on_before_hide callback", function() | ||
| local before_hide_spy = spy.new(function() end) | ||
| widget:set_on_before_hide(before_hide_spy) | ||
| widget:show() | ||
|
|
||
| widget:hide() | ||
|
|
||
| assert.is_true(before_hide_spy.call_count > 0) | ||
| end) |
There was a problem hiding this comment.
Assert the callback fires exactly once.
call_count > 0 will still pass if hide() regresses and invokes the hook multiple times during one close. This test should lock in the single-call contract explicitly.
🧪 Suggested fix
- assert.is_true(before_hide_spy.call_count > 0)
+ assert.spy(before_hide_spy).was.called(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lua/agentic/ui/chat_widget.test.lua` around lines 108 - 116, The test
currently asserts before_hide_spy.call_count > 0 which allows multiple
invocations; change the assertion to assert that the spy was called exactly once
to lock the contract. Locate the test which sets before_hide_spy via
widget:set_on_before_hide, calls widget:show() and widget:hide(), and replace
the final assertion with a strict equality check (use before_hide_spy.call_count
== 1 or the test framework's exact-call matcher) so the test fails if hide()
invokes the callback more than once.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lua/agentic/session_manager.lua (1)
320-328: 🧹 Nitpick | 🔵 TrivialUse
bufwinidfor guaranteed accuracy.
nvim_get_current_win()returns the current window at callback time, which should be the window the buffer just entered. However,vim.fn.bufwinid(self.widget.buf_nrs.chat)explicitly retrieves the window displaying the chat buffer, avoiding potential edge cases with rapid window switches.♻️ Suggested fix
vim.api.nvim_create_autocmd("BufWinEnter", { buffer = self.widget.buf_nrs.chat, callback = function() - local winid = vim.api.nvim_get_current_win() + local winid = vim.fn.bufwinid(self.widget.buf_nrs.chat) + if winid == -1 then + return + end self.chat_folds:on_buf_win_enter(winid) end, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/agentic/session_manager.lua` around lines 320 - 328, Replace the use of vim.api.nvim_get_current_win() in SessionManager:_bind_chat_buffer_events with a call to vim.fn.bufwinid(self.widget.buf_nrs.chat) to get the actual window showing the chat buffer; update the autocmd callback to compute winid = vim.fn.bufwinid(self.widget.buf_nrs.chat) and then call self.chat_folds:on_buf_win_enter(winid) so the handler always receives the correct window for the chat buffer even during rapid window switches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lua/agentic/session_manager.lua`:
- Around line 320-328: Replace the use of vim.api.nvim_get_current_win() in
SessionManager:_bind_chat_buffer_events with a call to
vim.fn.bufwinid(self.widget.buf_nrs.chat) to get the actual window showing the
chat buffer; update the autocmd callback to compute winid =
vim.fn.bufwinid(self.widget.buf_nrs.chat) and then call
self.chat_folds:on_buf_win_enter(winid) so the handler always receives the
correct window for the chat buffer even during rapid window switches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a9b66fa-cc64-4579-83ba-edb76d3d6f18
📒 Files selected for processing (3)
lua/agentic/session_manager.lualua/agentic/ui/chat_widget.lualua/agentic/ui/chat_widget.test.lua
elsesiy
left a comment
elsesiy
left a comment
There was a problem hiding this comment.
lgtm, @carlos-algms mind taking another look? 🙇
|
That's a very desirable feature, and quite a good amount of code. |
|
Thank you @jfahrer for the contribution 🫡 I've tried an alternative approach and managed to achieve a good result with fewer lines. It's already merged, I hope it covers your needs. |
## Summary
- Auto-fold tool call bodies whose interior exceeds a configurable line
threshold (default 10)
- Diff-kind tool calls are never folded (preserves diff context)
- New `agentic.ui.tool_call_fold` module owns fold logic (foldexpr,
foldtext, setup_window, register/unregister)
- Uses Neovim's `foldmethod=expr` with a getter closure over
MessageWriter's `tool_call_blocks`; no cache, Neovim re-evaluates on
buffer edits
- Config: `folding.tool_calls = { enabled = true, threshold = 10 }`
(threshold = 0 always folds, negatives clamp to 0, enabled = false
disables)
- Folded display: ` N lines hidden (zo open | zc close)`
- closes carlos-algms#154
4 participants
Fold completed tool output in the chat buffer so noisy responses stay compact while tool headers and status remain visible. Addresses #18
I was struggling with long tool output and noticed the open issue. I thought I let codex have a try to address this. The change was implemented by GPT 5.4 and reviewed by Claude Opus 4.6. I have not looked at the code in detail and have only tested it for a little while. I have no expectations for anyone to look at the code at this point. But a review of the design would be helpful to ensure this is going in the right direction. I test this out a bit more this week.
Some questions I wasn't sure about: