Use worktree qualified paths in agent file context + some code cleanup (#27943)

Michael Sloan created

Release Notes:

- N/A

Change summary

crates/agent/src/context_store.rs | 144 +++++++++++---------------------
crates/project/src/project.rs     |   8 
2 files changed, 53 insertions(+), 99 deletions(-)

Detailed changes

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>,
+        buffer: Entity<Buffer>,
         cx: &mut Context<Self>,
     ) -> Task<Result<()>> {
         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<Self>,
     ) -> Task<Result<bool>> {
         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<Buffer>,
-    file: Arc<dyn File>,
     id: BufferId,
+    buffer: Entity<Buffer>,
+    file: Arc<dyn File>,
     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<Path>,
-    buffer_entity: Entity<Buffer>,
-    buffer: &Buffer,
+    buffer: Entity<Buffer>,
     range: Option<Range<Anchor>>,
-    cx: AsyncApp,
+    cx: &App,
 ) -> Result<(BufferInfo, Task<SharedString>)> {
-    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::<Rope>()
+        buffer_ref.text_for_range(range).collect::<Rope>()
     } 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<impl Future<Output = ContextBuffer> + 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<impl Future<Output = ContextSymbol> + 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();

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,
@@ -4989,7 +4989,7 @@ impl ProjectItem for Buffer {
     }
 
     fn project_path(&self, cx: &App) -> Option<ProjectPath> {
-        File::from_dyn(self.file()).map(|file| ProjectPath {
+        self.file().map(|file| ProjectPath {
             worktree_id: file.worktree_id(cx),
             path: file.path().clone(),
         })