From 28baedd935295aa3219116a5b0b48b5c6370c848 Mon Sep 17 00:00:00 2001 From: Agus Zubiaga Date: Wed, 2 Jul 2025 18:39:43 -0300 Subject: [PATCH] Show tool output diffs --- Cargo.lock | 1 + crates/acp/Cargo.toml | 1 + crates/acp/src/acp.rs | 95 ++++++++++++++++--- crates/acp/src/thread_view.rs | 167 +++++++++++++++++++++++++++++----- 4 files changed, 229 insertions(+), 35 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d4e7d08dee3c88b763f990d29e3e258eb437d68d..d5bf270e5d194e9af3c27453d9961653ce8db3c3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10,6 +10,7 @@ dependencies = [ "anyhow", "async-trait", "base64 0.22.1", + "buffer_diff", "chrono", "collections", "editor", diff --git a/crates/acp/Cargo.toml b/crates/acp/Cargo.toml index 2b81dc03e997aaac44feb87fc7c8b65ee19a59f2..3992a06cdb757378a49eb90d310f6e9b5ca06f59 100644 --- a/crates/acp/Cargo.toml +++ b/crates/acp/Cargo.toml @@ -20,6 +20,7 @@ agentic-coding-protocol = { path = "../../../agentic-coding-protocol" } anyhow.workspace = true async-trait.workspace = true base64.workspace = true +buffer_diff.workspace = true chrono.workspace = true collections.workspace = true editor.workspace = true diff --git a/crates/acp/src/acp.rs b/crates/acp/src/acp.rs index 31d9f66dfad89a68c681eb0729d2888adfb3a5d1..429a596ec8219a3af8019b55fa8d6fab59d2d257 100644 --- a/crates/acp/src/acp.rs +++ b/crates/acp/src/acp.rs @@ -3,10 +3,12 @@ mod thread_view; use agentic_coding_protocol::{self as acp, Role}; use anyhow::{Context as _, Result}; +use buffer_diff::BufferDiff; use chrono::{DateTime, Utc}; +use editor::MultiBuffer; use futures::channel::oneshot; use gpui::{AppContext, Context, Entity, EventEmitter, SharedString, Task}; -use language::LanguageRegistry; +use language::{Buffer, LanguageRegistry}; use markdown::Markdown; use project::Project; use std::{mem, ops::Range, path::PathBuf, sync::Arc}; @@ -138,11 +140,24 @@ pub enum ToolCallStatus { Allowed { // todo! should this be variants in crate::ToolCallStatus instead? status: acp::ToolCallStatus, - content: Option>, + content: Option, }, Rejected, } +#[derive(Debug)] +pub enum ToolCallContent { + Markdown { + markdown: Entity, + }, + Diff { + path: PathBuf, + diff: Entity, + buffer: Entity, + _task: Task>, + }, +} + /// A `ThreadEntryId` that is known to be a ToolCall #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct ToolCallId(ThreadEntryId); @@ -375,14 +390,68 @@ impl AcpThread { match &mut entry.content { AgentThreadEntryContent::ToolCall(call) => match &mut call.status { ToolCallStatus::Allowed { content, status } => { - *content = new_content.map(|new_content| { - let acp::ToolCallContent::Markdown { markdown } = new_content; - - cx.new(|cx| { - Markdown::new(markdown.into(), Some(language_registry), None, cx) - }) + *content = new_content.map(|new_content| match new_content { + acp::ToolCallContent::Markdown { markdown } => ToolCallContent::Markdown { + markdown: cx.new(|cx| { + Markdown::new( + markdown.into(), + Some(language_registry.clone()), + None, + cx, + ) + }), + }, + acp::ToolCallContent::Diff { + path, + old_text, + new_text, + } => { + let buffer = cx.new(|cx| Buffer::local(new_text, cx)); + let text_snapshot = buffer.read(cx).text_snapshot(); + let buffer_diff = cx.new(|cx| BufferDiff::new(&text_snapshot, cx)); + + let multibuffer = cx.new(|cx| { + let mut multibuffer = MultiBuffer::singleton(buffer.clone(), cx); + multibuffer.add_diff(buffer_diff.clone(), cx); + multibuffer + }); + + ToolCallContent::Diff { + path: path.clone(), + diff: buffer_diff.clone(), + buffer: multibuffer, + _task: cx.spawn(async move |_this, cx| { + let diff_snapshot = BufferDiff::update_diff( + buffer_diff.clone(), + text_snapshot.clone(), + old_text.map(|o| o.into()), + true, + true, + None, + Some(language_registry.clone()), + cx, + ) + .await?; + + buffer_diff.update(cx, |diff, cx| { + diff.set_snapshot(diff_snapshot, &text_snapshot, cx) + })?; + + if let Some(language) = language_registry + .language_for_file_path(&path) + .await + .log_err() + { + buffer.update(cx, |buffer, cx| { + buffer.set_language(Some(language), cx) + })?; + } + + anyhow::Ok(()) + }), + } + } }); - *status = new_status; } ToolCallStatus::WaitingForConfirmation { .. } => { @@ -610,14 +679,18 @@ mod tests { thread.read_with(cx, |thread, cx| { let AgentThreadEntryContent::ToolCall(ToolCall { - status: ToolCallStatus::Allowed { content, .. }, + status: + ToolCallStatus::Allowed { + content: Some(ToolCallContent::Markdown { markdown }), + .. + }, .. }) = &thread.entries()[1].content else { panic!(); }; - content.as_ref().unwrap().read_with(cx, |md, _cx| { + markdown.read_with(cx, |md, _cx| { assert!( md.source().contains("Hello, world!"), r#"Expected '{}' to contain "Hello, world!""#, diff --git a/crates/acp/src/thread_view.rs b/crates/acp/src/thread_view.rs index 5dbeb57bc29cf885226b294ce8a9524ee21c6062..209e15aed290d995df95339d8967b0244aec58cd 100644 --- a/crates/acp/src/thread_view.rs +++ b/crates/acp/src/thread_view.rs @@ -4,7 +4,7 @@ use std::time::Duration; use agentic_coding_protocol::{self as acp, ToolCallConfirmation}; use anyhow::Result; -use editor::{Editor, MultiBuffer}; +use editor::{Editor, EditorMode, MinimapVisibility, MultiBuffer}; use gpui::{ Animation, AnimationExt, App, EdgesRefinement, Empty, Entity, Focusable, ListState, SharedString, StyleRefinement, Subscription, TextStyleRefinement, Transformation, @@ -12,6 +12,7 @@ use gpui::{ }; use gpui::{FocusHandle, Task}; use language::Buffer; +use language::language_settings::SoftWrap; use markdown::{HeadingLevelStyles, MarkdownElement, MarkdownStyle}; use project::Project; use settings::Settings as _; @@ -23,17 +24,23 @@ use zed_actions::agent::Chat; use crate::{ AcpServer, AcpThread, AcpThreadEvent, AgentThreadEntryContent, MessageChunk, Role, ThreadEntry, - ToolCall, ToolCallId, ToolCallStatus, + ToolCall, ToolCallContent, ToolCallId, ToolCallStatus, }; pub struct AcpThreadView { thread_state: ThreadState, - // todo! use full message editor from agent2 + // todo! reconsider structure. currently pretty sparse, but easy to clean up if we need to delete entries. + thread_entry_views: Vec>, message_editor: Entity, list_state: ListState, send_task: Option>>, } +#[derive(Debug)] +enum ThreadEntryView { + Diff { editor: Entity }, +} + enum ThreadState { Loading { _task: Task<()>, @@ -78,13 +85,14 @@ impl AcpThreadView { else { return Empty.into_any(); }; - this.render_entry(entry, window, cx) + this.render_entry(item, entry, window, cx) } }), ); Self { thread_state: Self::initial_state(project, window, cx), + thread_entry_views: Vec::new(), message_editor, send_task: None, list_state: list_state, @@ -126,21 +134,11 @@ impl AcpThreadView { let agent = AcpServer::stdio(child, project, cx); let result = agent.clone().create_thread(cx).await; - this.update(cx, |this, cx| { + this.update_in(cx, |this, window, cx| { match result { Ok(thread) => { - let subscription = cx.subscribe(&thread, |this, _, event, cx| { - let count = this.list_state.item_count(); - match event { - AcpThreadEvent::NewEntry => { - this.list_state.splice(count..count, 1); - } - AcpThreadEvent::EntryUpdated(index) => { - this.list_state.splice(*index..*index + 1, 1); - } - } - cx.notify(); - }); + let subscription = + cx.subscribe_in(&thread, window, Self::handle_thread_event); this.list_state .splice(0..0, thread.read(cx).entries().len()); @@ -212,6 +210,108 @@ impl AcpThreadView { }); } + fn handle_thread_event( + &mut self, + thread: &Entity, + event: &AcpThreadEvent, + window: &mut Window, + cx: &mut Context, + ) { + let count = self.list_state.item_count(); + match event { + AcpThreadEvent::NewEntry => { + self.sync_thread_entry_view(thread.read(cx).entries.len() - 1, window, cx); + self.list_state.splice(count..count, 1); + } + AcpThreadEvent::EntryUpdated(index) => { + let index = *index; + self.sync_thread_entry_view(index, window, cx); + self.list_state.splice(index..index + 1, 1); + } + } + cx.notify(); + } + + fn sync_thread_entry_view( + &mut self, + entry_ix: usize, + window: &mut Window, + cx: &mut Context, + ) { + let Some(buffer) = self.entry_diff_buffer(entry_ix, cx) else { + return; + }; + + if let Some(Some(ThreadEntryView::Diff { .. })) = self.thread_entry_views.get(entry_ix) { + return; + } + // todo! should we do this on the fly from render? + + let editor = cx.new(|cx| { + let mut editor = Editor::new( + EditorMode::Full { + scale_ui_elements_with_buffer_font_size: false, + show_active_line_background: false, + sized_by_content: true, + }, + buffer.clone(), + None, + window, + cx, + ); + editor.set_show_gutter(false, cx); + editor.disable_inline_diagnostics(); + editor.disable_expand_excerpt_buttons(cx); + editor.set_show_vertical_scrollbar(false, cx); + editor.set_minimap_visibility(MinimapVisibility::Disabled, window, cx); + editor.set_soft_wrap_mode(SoftWrap::None, cx); + editor.scroll_manager.set_forbid_vertical_scroll(true); + editor.set_show_indent_guides(false, cx); + editor.set_read_only(true); + editor.set_show_breakpoints(false, cx); + editor.set_show_code_actions(false, cx); + editor.set_show_git_diff_gutter(false, cx); + editor.set_expand_all_diff_hunks(cx); + editor.set_text_style_refinement(TextStyleRefinement { + font_size: Some( + TextSize::Small + .rems(cx) + .to_pixels(ThemeSettings::get_global(cx).agent_font_size(cx)) + .into(), + ), + ..Default::default() + }); + editor + }); + + if entry_ix >= self.thread_entry_views.len() { + self.thread_entry_views + .resize_with(entry_ix + 1, Default::default); + } + + self.thread_entry_views[entry_ix] = Some(ThreadEntryView::Diff { + editor: editor.clone(), + }); + } + + fn entry_diff_buffer(&self, entry_ix: usize, cx: &App) -> Option> { + let entry = self.thread()?.read(cx).entries().get(entry_ix)?; + + if let AgentThreadEntryContent::ToolCall(ToolCall { + status: + crate::ToolCallStatus::Allowed { + content: Some(ToolCallContent::Diff { buffer, .. }), + .. + }, + .. + }) = &entry.content + { + Some(buffer.clone()) + } else { + None + } + } + fn authorize_tool_call( &mut self, id: ToolCallId, @@ -229,6 +329,7 @@ impl AcpThreadView { fn render_entry( &self, + index: usize, entry: &ThreadEntry, window: &mut Window, cx: &Context, @@ -277,12 +378,18 @@ impl AcpThreadView { AgentThreadEntryContent::ToolCall(tool_call) => div() .px_2() .py_4() - .child(self.render_tool_call(tool_call, window, cx)) + .child(self.render_tool_call(index, tool_call, window, cx)) .into_any(), } } - fn render_tool_call(&self, tool_call: &ToolCall, window: &Window, cx: &Context) -> Div { + fn render_tool_call( + &self, + entry_ix: usize, + tool_call: &ToolCall, + window: &Window, + cx: &Context, + ) -> Div { let status_icon = match &tool_call.status { ToolCallStatus::WaitingForConfirmation { .. } => Empty.into_element().into_any(), ToolCallStatus::Allowed { @@ -318,16 +425,28 @@ impl AcpThreadView { ToolCallStatus::WaitingForConfirmation { confirmation, .. } => { Some(self.render_tool_call_confirmation(tool_call.id, confirmation, cx)) } - ToolCallStatus::Allowed { content, .. } => content.clone().map(|content| { + ToolCallStatus::Allowed { content, .. } => content.as_ref().map(|content| { div() .border_color(cx.theme().colors().border) .border_t_1() .px_2() .py_1p5() - .child(MarkdownElement::new( - content, - default_markdown_style(window, cx), - )) + .child(match content { + ToolCallContent::Markdown { markdown } => MarkdownElement::new( + markdown.clone(), + default_markdown_style(window, cx), + ) + .into_any_element(), + ToolCallContent::Diff { .. } => { + if let Some(Some(ThreadEntryView::Diff { editor })) = + self.thread_entry_views.get(entry_ix) + { + editor.clone().into_any_element() + } else { + Empty.into_any() + } + } + }) .into_any_element() }), ToolCallStatus::Rejected => None,