From b7b7f1ccdd63d66df6127010381e4edf112581ce Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Wed, 2 Apr 2025 12:00:32 -0600 Subject: [PATCH] Use worktree qualified paths in agent file context + some code cleanup (#27943) Release Notes: - N/A --- crates/agent/src/context_store.rs | 144 ++++++++++-------------------- crates/project/src/project.rs | 8 +- 2 files changed, 53 insertions(+), 99 deletions(-) diff --git a/crates/agent/src/context_store.rs b/crates/agent/src/context_store.rs index 009e638816..f152864154 100644 --- a/crates/agent/src/context_store.rs +++ b/crates/agent/src/context_store.rs @@ -6,7 +6,7 @@ use anyhow::{Context as _, Result, anyhow}; use collections::{BTreeMap, HashMap, HashSet}; use futures::future::join_all; use futures::{self, Future, FutureExt, future}; -use gpui::{App, AppContext as _, AsyncApp, Context, Entity, SharedString, Task, WeakEntity}; +use gpui::{App, AppContext as _, Context, Entity, SharedString, Task, WeakEntity}; use language::{Buffer, File}; use project::{ProjectItem, ProjectPath, Worktree}; use rope::Rope; @@ -95,8 +95,8 @@ impl ContextStore { project.open_buffer(project_path.clone(), cx) })?; - let buffer_entity = open_buffer_task.await?; - let buffer_id = this.update(cx, |_, cx| buffer_entity.read(cx).remote_id())?; + let buffer = open_buffer_task.await?; + let buffer_id = this.update(cx, |_, cx| buffer.read(cx).remote_id())?; let already_included = this.update(cx, |this, _cx| { match this.will_include_buffer(buffer_id, &project_path.path) { @@ -115,16 +115,8 @@ impl ContextStore { return anyhow::Ok(()); } - let (buffer_info, text_task) = this.update(cx, |_, cx| { - let buffer = buffer_entity.read(cx); - collect_buffer_info_and_text( - project_path.path.clone(), - buffer_entity, - buffer, - None, - cx.to_async(), - ) - })??; + let (buffer_info, text_task) = + this.update(cx, |_, cx| collect_buffer_info_and_text(buffer, None, cx))??; let text = text_task.await; @@ -138,23 +130,12 @@ impl ContextStore { pub fn add_file_from_buffer( &mut self, - buffer_entity: Entity, + buffer: Entity, cx: &mut Context, ) -> Task> { cx.spawn(async move |this, cx| { - let (buffer_info, text_task) = this.update(cx, |_, cx| { - let buffer = buffer_entity.read(cx); - let Some(file) = buffer.file() else { - return Err(anyhow!("Buffer has no path.")); - }; - collect_buffer_info_and_text( - file.path().clone(), - buffer_entity, - buffer, - None, - cx.to_async(), - ) - })??; + let (buffer_info, text_task) = + this.update(cx, |_, cx| collect_buffer_info_and_text(buffer, None, cx))??; let text = text_task.await; @@ -169,10 +150,8 @@ impl ContextStore { fn insert_file(&mut self, context_buffer: ContextBuffer) { let id = self.next_context_id.post_inc(); self.files.insert(context_buffer.id, id); - self.context.push(AssistantContext::File(FileContext { - id, - context_buffer: context_buffer, - })); + self.context + .push(AssistantContext::File(FileContext { id, context_buffer })); } pub fn add_directory( @@ -233,22 +212,13 @@ impl ContextStore { let mut buffer_infos = Vec::new(); let mut text_tasks = Vec::new(); this.update(cx, |_, cx| { - for (path, buffer_entity) in files.into_iter().zip(buffers) { - // Skip all binary files and other non-UTF8 files - if let Ok(buffer_entity) = buffer_entity { - let buffer = buffer_entity.read(cx); - if let Some((buffer_info, text_task)) = collect_buffer_info_and_text( - path, - buffer_entity, - buffer, - None, - cx.to_async(), - ) - .log_err() - { - buffer_infos.push(buffer_info); - text_tasks.push(text_task); - } + // Skip all binary files and other non-UTF8 files + for buffer in buffers.into_iter().flatten() { + if let Some((buffer_info, text_task)) = + collect_buffer_info_and_text(buffer, None, cx).log_err() + { + buffer_infos.push(buffer_info); + text_tasks.push(text_task); } } anyhow::Ok(()) @@ -298,12 +268,8 @@ impl ContextStore { cx: &mut Context, ) -> Task> { let buffer_ref = buffer.read(cx); - let Some(file) = buffer_ref.file() else { - return Task::ready(Err(anyhow!("Buffer has no path."))); - }; - let Some(project_path) = buffer_ref.project_path(cx) else { - return Task::ready(Err(anyhow!("Buffer has no project path."))); + return Task::ready(Err(anyhow!("buffer has no path"))); }; if let Some(symbols_for_path) = self.symbols_by_path.get(&project_path) { @@ -326,16 +292,11 @@ impl ContextStore { } } - let (buffer_info, collect_content_task) = match collect_buffer_info_and_text( - file.path().clone(), - buffer, - buffer_ref, - Some(symbol_enclosing_range.clone()), - cx.to_async(), - ) { - Ok((buffer_info, collect_context_task)) => (buffer_info, collect_context_task), - Err(err) => return Task::ready(Err(err)), - }; + let (buffer_info, collect_content_task) = + match collect_buffer_info_and_text(buffer, Some(symbol_enclosing_range.clone()), cx) { + Ok((buffer_info, collect_context_task)) => (buffer_info, collect_context_task), + Err(err) => return Task::ready(Err(err)), + }; cx.spawn(async move |this, cx| { let content = collect_content_task.await; @@ -616,16 +577,16 @@ pub enum FileInclusion { // ContextBuffer without text. struct BufferInfo { - buffer_entity: Entity, - file: Arc, id: BufferId, + buffer: Entity, + file: Arc, version: clock::Global, } fn make_context_buffer(info: BufferInfo, text: SharedString) -> ContextBuffer { ContextBuffer { id: info.id, - buffer: info.buffer_entity, + buffer: info.buffer, file: info.file, version: info.version, text, @@ -644,34 +605,37 @@ fn make_context_symbol( id: ContextSymbolId { name, range, path }, buffer_version: info.version, enclosing_range, - buffer: info.buffer_entity, + buffer: info.buffer, text, } } fn collect_buffer_info_and_text( - path: Arc, - buffer_entity: Entity, - buffer: &Buffer, + buffer: Entity, range: Option>, - cx: AsyncApp, + cx: &App, ) -> Result<(BufferInfo, Task)> { - let buffer_info = BufferInfo { - id: buffer.remote_id(), - buffer_entity, - file: buffer - .file() - .context("buffer context must have a file")? - .clone(), - version: buffer.version(), - }; + let buffer_ref = buffer.read(cx); + let file = buffer_ref.file().context("file context must have a path")?; + // Important to collect version at the same time as content so that staleness logic is correct. + let version = buffer_ref.version(); let content = if let Some(range) = range { - buffer.text_for_range(range).collect::() + buffer_ref.text_for_range(range).collect::() } else { - buffer.as_rope().clone() + buffer_ref.as_rope().clone() }; - let text_task = cx.background_spawn(async move { to_fenced_codeblock(&path, content) }); + + let buffer_info = BufferInfo { + buffer, + id: buffer_ref.remote_id(), + file: file.clone(), + version, + }; + + let full_path = file.full_path(cx); + let text_task = cx.background_spawn(async move { to_fenced_codeblock(&full_path, content) }); + Ok((buffer_info, text_task)) } @@ -920,16 +884,9 @@ fn refresh_context_buffer( cx: &App, ) -> Option + use<>> { let buffer = context_buffer.buffer.read(cx); - let path = buffer_path_log_err(buffer, cx)?; if buffer.version.changed_since(&context_buffer.version) { - let (buffer_info, text_task) = collect_buffer_info_and_text( - path, - context_buffer.buffer.clone(), - buffer, - None, - cx.to_async(), - ) - .log_err()?; + let (buffer_info, text_task) = + collect_buffer_info_and_text(context_buffer.buffer.clone(), None, cx).log_err()?; Some(text_task.map(move |text| make_context_buffer(buffer_info, text))) } else { None @@ -941,15 +898,12 @@ fn refresh_context_symbol( cx: &App, ) -> Option + use<>> { let buffer = context_symbol.buffer.read(cx); - let path = buffer_path_log_err(buffer, cx)?; let project_path = buffer.project_path(cx)?; if buffer.version.changed_since(&context_symbol.buffer_version) { let (buffer_info, text_task) = collect_buffer_info_and_text( - path, context_symbol.buffer.clone(), - buffer, Some(context_symbol.enclosing_range.clone()), - cx.to_async(), + cx, ) .log_err()?; let name = context_symbol.id.name.clone(); diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index 1824e91e57..2801c3cd93 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -63,9 +63,9 @@ use gpui::{ }; use itertools::Itertools; use language::{ - Buffer, BufferEvent, Capability, CodeLabel, File as _, Language, LanguageName, - LanguageRegistry, PointUtf16, ToOffset, ToPointUtf16, Toolchain, ToolchainList, Transaction, - Unclipped, language_settings::InlayHintKind, proto::split_operations, + Buffer, BufferEvent, Capability, CodeLabel, Language, LanguageName, LanguageRegistry, + PointUtf16, ToOffset, ToPointUtf16, Toolchain, ToolchainList, Transaction, Unclipped, + language_settings::InlayHintKind, proto::split_operations, }; use lsp::{ CodeActionKind, CompletionContext, CompletionItemKind, DocumentHighlightKind, LanguageServerId, @@ -4990,7 +4990,7 @@ impl ProjectItem for Buffer { } fn project_path(&self, cx: &App) -> Option { - File::from_dyn(self.file()).map(|file| ProjectPath { + self.file().map(|file| ProjectPath { worktree_id: file.worktree_id(cx), path: file.path().clone(), })