diff --git a/AGENTS.md b/AGENTS.md index a5d612af..1ebae5e6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -5,31 +5,23 @@ Additional documentation is available in the `docs` folder if needed. ## Essential Commands -### Building and Development - `cargo check` - Test if the project compiles - `cargo check --tests` - Test if the tests compile - `cargo test` - Run all tests +- `cargo test --package ` - Test a single crate (names in Crate Layers below) - `cargo fmt --all -- --check` - Check code formatting - `cargo clippy --all-targets --all-features -- -D warnings` - Run linter -### Testing Specific Components -- `cargo test --package code-assistant` - Test wiring binary -- `cargo test --package code_assistant_core` - Test domain layer -- `cargo test --package agent_core` - Test agent core -- `cargo test --package tools_core` - Test tool framework -- `cargo test --package llm` - Test LLM integration -- `cargo test --package web` - Test web functionality - ## Architecture Overview -This is a Rust-based tool for AI-assisted code tasks with multiple operational modes. +This is a Rust-based AI coding agent harness with multiple operational modes. ### Crate Layers ``` Layer 0 (generic): llm command_executor pty_session fs_explorer sandbox web git terminal terminal_output Layer 1 (generic): tools_core — tool trait, registry, render, spec, permissions - mcp_client — MCP client mode: wraps MCP server tools as registry tools (rmcp SDK) + mcp_client — wraps MCP server tools as registry tools Layer 2 (generic): agent_core — agent loop, hook traits, dialect trait, AgentUi trait Layer 3 (domain): code_assistant_core — sessions, SessionService, event stream, UiEvent, tool impls, dialects (xml/caret), plugins, sub-agents @@ -37,27 +29,17 @@ Layer 4 (frontends): ui_gpui ui_terminal ui_acp mcp_server Layer 5 (binary): code_assistant — CLI, config, feature-gated frontend wiring ``` -The binary feature-gates `gpui-frontend`, `terminal-frontend`, `acp-frontend`, -and `mcp-server` (all default). A `--no-default-features` build produces a -headless binary without gpui. - ### Key Entry Points - **Agent loop**: `crates/agent_core/src/runtime.rs` - **Domain agent wrapper**: `crates/code_assistant_core/src/agent/runner.rs` -- **Tool trait & registry**: `crates/tools_core/src/` - **Tool implementations**: `crates/code_assistant_core/src/tools/` - **Tool dialects (xml/caret)**: `crates/code_assistant_core/src/tool_dialects/` - **Plugins/hooks**: `crates/code_assistant_core/src/plugins/` - **Session management**: `crates/code_assistant_core/src/session/` -- **GPUI frontend**: `crates/ui_gpui/src/` -- **Terminal frontend**: `crates/ui_terminal/src/` -- **MCP server**: `crates/mcp_server/src/` -- **MCP client**: `crates/mcp_client/src/` (config loading/registration binding: `crates/code_assistant_core/src/tools/mcp.rs`) -- **ACP frontend**: `crates/ui_acp/src/` +- **MCP client config/registration binding**: `crates/code_assistant_core/src/tools/mcp.rs` ### Tool Architecture -- **Core framework** (`tools_core`): `DynTool` trait, `ToolRegistry` (instance, not singleton), `ToolSpec` with capability tags -- **Tool implementations** live in `code_assistant_core::tools` +- `ToolRegistry` is an instance, not a singleton; `ToolSpec` carries capability tags (e.g. `read_only`) - **Tool modes** (configured per agent instance via `ToolDialect`): - `native` — LLM provider's native tool calling (default in `agent_core`) - `xml` — XML-based tool syntax in system messages @@ -70,43 +52,21 @@ headless binary without gpui. ## Configuration -### MCP Server Mode -- Integrates with Claude Desktop as MCP server - ### MCP Client Mode -- Connects to configured MCP servers over stdio (child process) or HTTP - (streamable transport, official `rmcp` SDK) and registers their tools in - the `ToolRegistry` as `mcp____` with scope tags - `scope:agent`/`scope:agent-diff` plus `mcp` and `scope:mcp-` -- Configured in `/mcp-servers.json` (per-server `enabled`, - `enabled_tools` allowlist, `disabled_tools` denylist; a stdio server has - `command`/`args`/`env`, an HTTP server has `url`/`headers`; `${ENV_VAR}` - substitution in `env`/`headers` values) or programmatically via - `mcp_client::register_mcp_tools` -- Projects can ship additional servers in a `.mcp.json` at the project root - (Claude Code's format), merged over the global set per project and gated - by a trust prompt — see `docs/project-scoped-mcp-servers.md` -- The tool registry is rebuilt from the current config at the start of - every agent run via the `ToolRegistryProvider` seam - (`tools::ConfigToolRegistry`, a fingerprint cache over `tools.json` + - `mcp-servers.json`; wired in all three frontends). MCP config changes - therefore apply on the next run without a restart; a running agent keeps - the registry it started with. The gpui settings page ("MCP Servers") - edits the file and discovers tools over ephemeral connections - -### Agent Mode -- Supports terminal, Agent Client Protocol, and GPUI interfaces -- State persistence for continuing sessions +- Connects to MCP servers configured in `/mcp-servers.json` + (stdio or HTTP, via the `rmcp` SDK) and registers their tools in the + `ToolRegistry` as `mcp____`; projects can add servers in a + `.mcp.json` at the project root +- The registry is rebuilt from the current config at the start of every + agent run (`ToolRegistryProvider` seam) +- Details: `docs/mcp-client-mode.md`, `docs/project-scoped-mcp-servers.md` ### Permission Tiers - Per-session setting deciding when the agent asks before running a tool: - `bypass-all` (default, never ask), `write-tools` (ask for anything not - tagged `read_only`), `all-tools` (ask always); see `docs/permission-tiers.md` -- Gate lives in the agent loop (`tools_core::ToolPermissions`); prompts - travel through the `PermissionMediator` seam — event-stream mediator for - GPUI/terminal (`SessionService::respond_permission`), ACP's native - `requestPermission` otherwise; tiers are exposed to ACP clients as - session modes + `bypass-all` (default), `write-tools` (ask for anything not tagged + `read_only`), `all-tools` (ask always) +- Gate lives in the agent loop (`tools_core::ToolPermissions`); prompts go + through the `PermissionMediator` seam; see `docs/permission-tiers.md` ## Development Notes @@ -117,165 +77,37 @@ headless binary without gpui. ### UI Development - GPUI frontend based on Zed's gpui and gpui-component with custom components -- Streaming processors per dialect in `code_assistant_core::tool_dialects/{xml,caret}/stream.rs` -- Theme support - -### Tool Development -- Implement `DynTool` / `Tool` traits from `tools_core` -- Register in a `ToolRegistry` instance via `register_default_tools()` in `code_assistant_core` -- Capability tags (e.g. `read_only`, `edits_files`) replace the old `ToolScope` enum +- GPUI API reference (contexts, entities, tasks, elements, actions, events): + `docs/gpui-reference.md` ## UI Communication Architecture Two directions across one seam (`code_assistant_core::session`): -1. **UI → core: `SessionService`** (`session/service.rs`) — every command a - frontend issues (create/load/delete session, send/queue message, switch - model/sandbox/worktree, branching, skills, `request_stop`) is a typed async - method returning `Result`. Internally an actor: methods enqueue a - closure on a command channel and await a oneshot reply; a single worker - (spawned on the backend tokio runtime by the wiring) executes commands in - order. `load_session` returns an owned `SessionSnapshot` (transcript incl. - in-flight partial response, tool results, plan, activity, model/sandbox - state); `SessionSnapshot::connect_events()` renders it as the canonical - event sequence. +1. **UI → core: `SessionService`** (`session/service.rs`) — every frontend + command is a typed async method. Internally an actor: a single worker on + the backend tokio runtime executes commands in order. `load_session` + returns an owned `SessionSnapshot`; `connect_events()` renders it as the + canonical event sequence. 2. **Core → UI: broadcast `EventStream`** (`session/event_stream.rs`) — all - notifications (streaming `DisplayFragment`s, `UiEvent`s) are published - session-tagged; frontends `subscribe()` and filter by the session they - view (sidebar-relevant events like activity/metadata pass regardless). - A lagged subscriber gets `StreamError::Lagged` and resyncs via a fresh - snapshot. The core does not know which session is "connected" or how many - views exist. - -### Concurrent Agent System -- **Multiple agents** can run concurrently, one per session; any number of - frontends/views can observe them via the stream -- **`SessionEventPublisher`** (`session/instance.rs`) implements the - `UserInterface` trait for the agent seam: it publishes everything and - records per-session in-flight state (fragments of the streaming response, - live tool statuses) that snapshots include; activity-state transition rules - live in `SessionActivity` -- **Cancellation** is a core-side per-session flag (`request_stop`), checked - by the agent at streaming checkpoints — works for background sessions too - -### Frontend patterns -- **GPUI**: commands in `ui_gpui/src/app/commands.rs` (dispatched on the - background executor), stream ingestion in `app/event_bridge.rs` -- **Terminal**: commands via the `Actions` struct, bridge task in - `ui_terminal/src/app.rs` -- **ACP**: routes stream events to per-prompt `ACPUserUI` instances via its - `active_uis` registry (`ui_acp/src/app.rs`); its session/prompt commands - intentionally use `SessionManager` directly — the protocol-adapter needs - (client-specified session ids, per-prompt agent starts, completion waiting) - don't map onto `SessionService` + notifications are published session-tagged; frontends `subscribe()` and + filter by the session they view. A lagged subscriber gets + `StreamError::Lagged` and resyncs via a fresh snapshot. The core does not + know which session is "connected" or how many views exist. + +Consequences: +- Multiple agents run concurrently, one per session; `SessionEventPublisher` + (`session/instance.rs`) implements the agent's `UserInterface` and records + the in-flight state that snapshots include +- Cancellation is a core-side per-session flag (`request_stop`), checked by + the agent at streaming checkpoints +- ACP's session/prompt commands intentionally bypass `SessionService` and + use `SessionManager` directly (protocol-adapter needs don't map onto it) - The filesystem `SessionWatcher` still pushes `UiEvent`s directly into - frontend channels (not via the stream) — a known remaining seam - -(Below instructions copied from Zed's `.rules` file) - -## GPUI - -GPUI is a UI framework which also provides primitives for state and concurrency management. - -### Context - -Context types allow interaction with global state, windows, entities, and system services. They are typically passed to functions as the argument named `cx`. When a function takes callbacks they come after the `cx` parameter. - -* `App` is the root context type, providing access to global state and read and update of entities. -* `Context` is provided when updating an `Entity`. This context dereferences into `App`, so functions which take `&App` can also take `&Context`. -* `AsyncApp` and `AsyncWindowContext` are provided by `cx.spawn` and `cx.spawn_in`. These can be held across await points. - -### `Window` - -`Window` provides access to the state of an application window. It is passed to functions as an argument named `window` and comes before `cx` when present. It is used for managing focus, dispatching actions, directly drawing, getting user input state, etc. - -### Entities - -An `Entity` is a handle to state of type `T`. With `thing: Entity`: - -* `thing.entity_id()` returns `EntityId` -* `thing.downgrade()` returns `WeakEntity` -* `thing.read(cx: &App)` returns `&T`. -* `thing.read_with(cx, |thing: &T, cx: &App| ...)` returns the closure's return value. -* `thing.update(cx, |thing: &mut T, cx: &mut Context| ...)` allows the closure to mutate the state, and provides a `Context` for interacting with the entity. It returns the closure's return value. -* `thing.update_in(cx, |thing: &mut T, window: &mut Window, cx: &mut Context| ...)` takes a `AsyncWindowContext` or `VisualTestContext`. It's the same as `update` while also providing the `Window`. - -Within the closures, the inner `cx` provided to the closure must be used instead of the outer `cx` to avoid issues with multiple borrows. - -Trying to update an entity while it's already being updated must be avoided as this will cause a panic. - -When `read_with`, `update`, or `update_in` are used with an async context, the closure's return value is wrapped in an `anyhow::Result`. - -`WeakEntity` is a weak handle. It has `read_with`, `update`, and `update_in` methods that work the same, but always return an `anyhow::Result` so that they can fail if the entity no longer exists. This can be useful to avoid memory leaks - if entities have mutually recursive handles to each other they will never be dropped. - -### Concurrency - -All use of entities and UI rendering occurs on a single foreground thread. - -`cx.spawn(async move |cx| ...)` runs an async closure on the foreground thread. Within the closure, `cx` is an async context like `AsyncApp` or `AsyncWindowContext`. - -When the outer cx is a `Context`, the use of `spawn` instead looks like `cx.spawn(async move |handle, cx| ...)`, where `handle: WeakEntity`. - -To do work on other threads, `cx.background_spawn(async move { ... })` is used. Often this background task is awaited on by a foreground task which uses the results to update state. - -Both `cx.spawn` and `cx.background_spawn` return a `Task`, which is a future that can be awaited upon. If this task is dropped, then its work is cancelled. To prevent this one of the following must be done: - -* Awaiting the task in some other async context. -* Detaching the task via `task.detach()` or `task.detach_and_log_err(cx)`, allowing it to run indefinitely. -* Storing the task in a field, if the work should be halted when the struct is dropped. - -A task which doesn't do anything but provide a value can be created with `Task::ready(value)`. - -### Elements - -The `Render` trait is used to render some state into an element tree that is laid out using flexbox layout. An `Entity` where `T` implements `Render` is sometimes called a "view". - -Example: - -``` -struct TextWithBorder(SharedString); - -impl Render for TextWithBorder { - fn render(&mut self, _window: &mut Window, _cx: &mut Context) -> impl IntoElement { - div().border_1().child(self.0.clone()) - } -} -``` - -Since `impl IntoElement for SharedString` exists, it can be used as an argument to `child`. `SharedString` is used to avoid copying strings, and is either an `&'static str` or `Arc`. - -UI components that are constructed just to be turned into elements can instead implement the `RenderOnce` trait, which is similar to `Render`, but its `render` method takes ownership of `self`. Types that implement this trait can use `#[derive(IntoElement)]` to use them directly as children. - -The style methods on elements are similar to those used by Tailwind CSS. - -If some attributes or children of an element tree are conditional, `.when(condition, |this| ...)` can be used to run the closure only when `condition` is true. Similarly, `.when_some(option, |this, value| ...)` runs the closure when the `Option` has a value. - -### Input events - -Input event handlers can be registered on an element via methods like `.on_click(|event, window, cx: &mut App| ...)`. - -Often event handlers will want to update the entity that's in the current `Context`. The `cx.listener` method provides this - its use looks like `.on_click(cx.listener(|this: &mut T, event, window, cx: &mut Context| ...)`. - -### Actions - -Actions are dispatched via user keyboard interaction or in code via `window.dispatch_action(SomeAction.boxed_clone(), cx)` or `focus_handle.dispatch_action(&SomeAction, window, cx)`. - -Actions with no data defined with the `actions!(some_namespace, [SomeAction, AnotherAction])` macro call. Otherwise the `Action` derive macro is used. Doc comments on actions are displayed to the user. - -Action handlers can be registered on an element via the event handler `.on_action(|action, window, cx| ...)`. Like other event handlers, this is often used with `cx.listener`. - -### Notify - -When a view's state has changed in a way that may affect its rendering, it should call `cx.notify()`. This will cause the view to be rerendered. It will also cause any observe callbacks registered for the entity with `cx.observe` to be called. - -### Entity events - -While updating an entity (`cx: Context`), it can emit an event using `cx.emit(event)`. Entities register which events they can emit by declaring `impl EventEmittor for EntityType {}`. - -Other entities can then register a callback to handle these events by doing `cx.subscribe(other_entity, |this, other_entity, event, cx| ...)`. This will return a `Subscription` which deregisters the callback when dropped. Typically `cx.subscribe` happens when creating a new entity and the subscriptions are stored in a `_subscriptions: Vec` field. + frontend channels, not via the stream — a known remaining seam -### Recent API changes +## GPUI API rules GPUI has had some changes to its APIs. Always write code using the new APIs: diff --git a/Cargo.lock b/Cargo.lock index 9b18671b..bc65a50a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3559,6 +3559,7 @@ version = "0.1.0" dependencies = [ "anyhow", "gix", + "notify", "serde", "tempfile", "tokio", diff --git a/crates/git/Cargo.toml b/crates/git/Cargo.toml index 702200da..ea888db3 100644 --- a/crates/git/Cargo.toml +++ b/crates/git/Cargo.toml @@ -5,6 +5,7 @@ edition = "2024" [dependencies] anyhow = "1.0.95" +notify = { version = "7", features = ["macos_fsevent"] } serde = { version = "1.0.215", features = ["derive"] } tokio = { version = "1.40.0", features = ["fs", "process"] } tracing = "0.1.40" diff --git a/crates/git/src/diff.rs b/crates/git/src/diff.rs index 55c3fa6b..9d8ad085 100644 --- a/crates/git/src/diff.rs +++ b/crates/git/src/diff.rs @@ -29,6 +29,13 @@ pub struct ChangedFile { pub orig_path: Option, /// The kind of change. pub status: ChangeStatus, + /// Opaque token that changes whenever either side of this file's diff may + /// have changed (blob ids in branch mode; `HEAD` plus the working-tree + /// file's mtime/size in working-tree mode). Lets consumers detect that a + /// previously loaded diff is stale without re-diffing. `None` when the + /// listing came from a source without fingerprints (e.g. an old cache). + #[serde(default)] + pub fingerprint: Option, } /// Aggregate line-change counts for a review listing (à la `git diff --stat`). @@ -95,7 +102,33 @@ impl GitRepository { ], ) .await?; - Ok(parse_status_z(&out)) + let mut files = parse_status_z(&out); + + // Fingerprint: HEAD (the old side of every diff) plus the working-tree + // file's mtime and size (the new side). Cheap — one stat per changed + // file — and precise enough to notice edits made while a diff is shown. + let head = self + .repo + .to_thread_local() + .head_id() + .map(|id| id.to_string()) + .unwrap_or_else(|_| "unborn".to_owned()); + for file in &mut files { + let new_side = match tokio::fs::metadata(self.workdir().join(&file.path)).await { + Ok(meta) => { + let mtime = meta + .modified() + .ok() + .and_then(|t| t.duration_since(std::time::UNIX_EPOCH).ok()) + .map(|d| d.as_nanos()) + .unwrap_or(0); + format!("{mtime}:{}", meta.len()) + } + Err(_) => "absent".to_owned(), + }; + file.fingerprint = Some(format!("{head}:{new_side}")); + } + Ok(files) } /// List files that differ between `base` and `HEAD` using three-dot @@ -110,14 +143,15 @@ impl GitRepository { "-c", "core.quotepath=false", "diff", - "--name-status", + "--raw", + "--no-abbrev", "-M", "-z", &range, ], ) .await?; - Ok(parse_diff_name_status_z(&out)) + Ok(parse_diff_raw_z(&out)) } /// Load both sides of the diff for `file` in working-tree mode: @@ -304,29 +338,42 @@ fn parse_status_z(bytes: &[u8]) -> Vec { path, orig_path, status, + fingerprint: None, }); } files } -/// Parse the output of `git diff --name-status -M -z`. +/// Parse the output of `git diff --raw --no-abbrev -M -z`. /// -/// Fields are NUL-terminated. A regular record is `STATUS`, then `PATH`. A -/// rename/copy record is `STATUS`, then the source path, then the destination -/// path. -fn parse_diff_name_status_z(bytes: &[u8]) -> Vec { +/// Fields are NUL-terminated. A record starts with a header +/// `: [score]`, followed by +/// `PATH` — or, for renames/copies, the source path and then the destination +/// path. The two blob ids make up the file's fingerprint. +fn parse_diff_raw_z(bytes: &[u8]) -> Vec { let mut files = Vec::new(); let mut chunks = bytes.split(|&b| b == 0).filter(|c| !c.is_empty()); - while let Some(status_chunk) = chunks.next() { - let code = status_chunk[0] as char; - let status = match code { - 'A' => ChangeStatus::Added, - 'D' => ChangeStatus::Deleted, - 'R' => ChangeStatus::Renamed, - 'C' => ChangeStatus::Copied, - 'T' => ChangeStatus::TypeChanged, + while let Some(header) = chunks.next() { + let header = String::from_utf8_lossy(header); + let mut fields = header.trim_start_matches(':').split(' '); + let (Some(_old_mode), Some(_new_mode), Some(old_blob), Some(new_blob), Some(status)) = ( + fields.next(), + fields.next(), + fields.next(), + fields.next(), + fields.next(), + ) else { + break; + }; + let status = match status.chars().next() { + Some('A') => ChangeStatus::Added, + Some('D') => ChangeStatus::Deleted, + Some('R') => ChangeStatus::Renamed, + Some('C') => ChangeStatus::Copied, + Some('T') => ChangeStatus::TypeChanged, _ => ChangeStatus::Modified, }; + let fingerprint = Some(format!("{old_blob}:{new_blob}")); if matches!(status, ChangeStatus::Renamed | ChangeStatus::Copied) { let Some(old) = chunks.next() else { break }; let Some(new) = chunks.next() else { break }; @@ -334,6 +381,7 @@ fn parse_diff_name_status_z(bytes: &[u8]) -> Vec { path: String::from_utf8_lossy(new).into_owned(), orig_path: Some(String::from_utf8_lossy(old).into_owned()), status, + fingerprint, }); } else { let Some(path) = chunks.next() else { break }; @@ -341,6 +389,7 @@ fn parse_diff_name_status_z(bytes: &[u8]) -> Vec { path: String::from_utf8_lossy(path).into_owned(), orig_path: None, status, + fingerprint, }); } } @@ -459,17 +508,73 @@ mod tests { } #[test] - fn parse_diff_name_status_handles_rename() { - let raw = b"M\0a.txt\0R100\0old.txt\0new.txt\0A\0added.txt\0"; - let files = parse_diff_name_status_z(raw); + fn parse_diff_raw_handles_rename_and_fingerprints() { + let raw = b":100644 100644 aaa bbb M\0a.txt\0\ + :100644 100644 ccc ccc R100\0old.txt\0new.txt\0\ + :000000 100644 000 ddd A\0added.txt\0"; + let files = parse_diff_raw_z(raw); assert_eq!(files.len(), 3); assert_eq!(files[0].status, ChangeStatus::Modified); assert_eq!(files[0].path, "a.txt"); + assert_eq!(files[0].fingerprint.as_deref(), Some("aaa:bbb")); assert_eq!(files[1].status, ChangeStatus::Renamed); assert_eq!(files[1].orig_path.as_deref(), Some("old.txt")); assert_eq!(files[1].path, "new.txt"); + assert_eq!(files[1].fingerprint.as_deref(), Some("ccc:ccc")); assert_eq!(files[2].status, ChangeStatus::Added); assert_eq!(files[2].path, "added.txt"); + assert_eq!(files[2].fingerprint.as_deref(), Some("000:ddd")); + } + + #[tokio::test] + async fn working_tree_fingerprint_tracks_content_changes() { + let dir = TempDir::new().unwrap(); + init_repo_with_commit(dir.path()); + + write(dir.path(), "a.txt", b"a\n"); + write(dir.path(), "b.txt", b"b\n"); + write(dir.path(), "gone.txt", b"gone\n"); + git(dir.path(), &["add", "."]); + git(dir.path(), &["commit", "-m", "seed"]); + + write(dir.path(), "a.txt", b"a changed\n"); + write(dir.path(), "b.txt", b"b changed\n"); + std::fs::remove_file(dir.path().join("gone.txt")).unwrap(); + + let repo = GitRepository::open(dir.path()).unwrap(); + let first = repo.changed_files_working_tree().await.unwrap(); + let a1 = find(&first, "a.txt") + .fingerprint + .clone() + .expect("fingerprint"); + let b1 = find(&first, "b.txt") + .fingerprint + .clone() + .expect("fingerprint"); + assert!(find(&first, "gone.txt").fingerprint.is_some()); + + // Only a.txt changes again (different size, so mtime granularity is + // irrelevant): its fingerprint moves, b.txt's stays put. + write(dir.path(), "a.txt", b"a changed once more\n"); + let second = repo.changed_files_working_tree().await.unwrap(); + assert_ne!( + find(&second, "a.txt").fingerprint.as_deref(), + Some(a1.as_str()) + ); + assert_eq!( + find(&second, "b.txt").fingerprint.as_deref(), + Some(b1.as_str()) + ); + + // A new HEAD changes the old side of every diff: b.txt is untouched in + // the working tree, yet its fingerprint must move. + git(dir.path(), &["add", "a.txt"]); + git(dir.path(), &["commit", "-m", "a only"]); + let third = repo.changed_files_working_tree().await.unwrap(); + assert_ne!( + find(&third, "b.txt").fingerprint.as_deref(), + Some(b1.as_str()) + ); } #[test] @@ -651,6 +756,26 @@ mod tests { .unwrap(); assert_eq!(d.old_text, None); assert_eq!(d.new_text.as_deref(), Some("new on feature\n")); + + // Fingerprints follow the blobs: another commit touching only + // shared.txt moves its fingerprint and leaves feature_only.txt's alone. + let shared1 = find(&files, "shared.txt").fingerprint.clone().unwrap(); + let only1 = find(&files, "feature_only.txt") + .fingerprint + .clone() + .unwrap(); + write(dir.path(), "shared.txt", b"feature line 2\n"); + git(dir.path(), &["add", "."]); + git(dir.path(), &["commit", "-m", "more feature work"]); + let files = repo.changed_files_vs_base(&base_branch).await.unwrap(); + assert_ne!( + find(&files, "shared.txt").fingerprint.as_deref(), + Some(shared1.as_str()) + ); + assert_eq!( + find(&files, "feature_only.txt").fingerprint.as_deref(), + Some(only1.as_str()) + ); } #[tokio::test] diff --git a/crates/git/src/lib.rs b/crates/git/src/lib.rs index 54abc87e..1a19bcf2 100644 --- a/crates/git/src/lib.rs +++ b/crates/git/src/lib.rs @@ -3,6 +3,7 @@ mod branch; mod diff; mod repository; mod types; +mod watch; pub mod worktree; pub use binary::GitBinary; @@ -10,6 +11,7 @@ pub use branch::BranchNotMerged; pub use diff::*; pub use repository::GitRepository; pub use types::*; +pub use watch::ChangeWatcher; #[cfg(test)] pub(crate) mod testutil { diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index e95451ad..06f10206 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -79,6 +79,12 @@ impl GitRepository { &self.workdir } + /// Return this worktree's private git directory: `.git` for the main + /// worktree, `
/.git/worktrees/` for a linked one. + pub fn gitdir(&self) -> PathBuf { + self.repo.to_thread_local().git_dir().to_path_buf() + } + /// Return the common directory (shared across worktrees). /// /// For the main worktree this is the same as `git_dir`. diff --git a/crates/git/src/watch.rs b/crates/git/src/watch.rs new file mode 100644 index 00000000..18f2e7d1 --- /dev/null +++ b/crates/git/src/watch.rs @@ -0,0 +1,347 @@ +//! Watch git working directories for changes that could alter a review. +//! +//! A [`ChangeWatcher`] watches each repo's working directory recursively (plus +//! the git dirs of linked worktrees, which live outside it) and calls back at +//! most once per quiet period. It answers only "something may have changed" — +//! the consumer re-lists and uses fingerprints to find out what. +//! +//! Events inside a `.git` directory are filtered the way Zed's worktree +//! scanner does: churn that never changes what `git status` or `git diff` +//! report (object writes, hooks, reflogs, lock files, temp files, …) is +//! dropped, while `index`, `HEAD`, refs and the like go through. + +use anyhow::{Context, Result}; +use notify::{Event, RecommendedWatcher, RecursiveMode, Watcher}; +use std::path::{Path, PathBuf}; +use std::sync::mpsc; +use std::time::{Duration, Instant}; +use tracing::{debug, trace, warn}; + +/// Files in a git dir whose changes never affect status or diffs. +const SKIPPED_FILE_NAMES_IN_DOT_GIT: [&str; 5] = [ + "COMMIT_EDITMSG", + "FETCH_HEAD", + "ORIG_HEAD", + "BISECT_LOG", + "gc.pid", +]; + +/// Subdirectories of a git dir whose churn never affects status or diffs. +const SKIPPED_DIRS_IN_DOT_GIT: [&str; 7] = [ + "fsmonitor--daemon", + "lfs", + "objects", + "hooks", + "rebase-merge", + "rebase-apply", + "sequencer", +]; + +const LOGS_DIR: &str = "logs"; +const LOGS_REF_STASH: &str = "logs/refs/stash"; +const INFO_DIR: &str = "info"; +const REPO_EXCLUDE: &str = "info/exclude"; + +/// Watches repositories and reports "something may have changed". +/// +/// Dropping the watcher stops it; a callback already in progress finishes. +pub struct ChangeWatcher { + _watcher: RecommendedWatcher, +} + +/// What to watch for one repository, and which directories count as git +/// dirs for event filtering. +#[derive(Debug, Clone, PartialEq, Eq)] +struct WatchTargets { + /// Directories to watch recursively. + paths: Vec, + /// Git dirs (private + common) — events under these are filtered. + git_dirs: Vec, +} + +impl ChangeWatcher { + /// Start watching `repo_roots`. `on_change` runs on a background thread + /// once a burst of events has been quiet for `debounce` — or after + /// `4 × debounce` of continuous activity, so a long build still yields + /// periodic refreshes. + pub fn start( + repo_roots: &[PathBuf], + debounce: Duration, + on_change: impl Fn() + Send + 'static, + ) -> Result { + let mut paths = Vec::new(); + let mut git_dirs = Vec::new(); + for root in repo_roots { + let targets = watch_targets(root)?; + paths.extend(targets.paths); + git_dirs.extend(targets.git_dirs); + } + + // Each interesting event is one `()` on this channel; the debounce + // thread turns bursts into single callbacks. Dropping the notify + // watcher drops the sender, which ends the thread. + let (tx, rx) = mpsc::channel::<()>(); + let filter_git_dirs = git_dirs.clone(); + let mut watcher = + notify::recommended_watcher(move |res: Result| match res { + Ok(event) => { + if event.paths.iter().any(|p| !is_noise(p, &filter_git_dirs)) { + trace!("Review watcher: relevant event {:?}", event); + let _ = tx.send(()); + } + } + Err(e) => warn!("Review watcher error: {e}"), + }) + .context("Failed to create filesystem watcher")?; + + for path in &paths { + watcher + .watch(path, RecursiveMode::Recursive) + .with_context(|| format!("Failed to watch {}", path.display()))?; + debug!("Review watcher: watching {}", path.display()); + } + + std::thread::Builder::new() + .name("review-change-watcher".into()) + .spawn(move || debounce_loop(rx, debounce, on_change)) + .context("Failed to spawn watcher debounce thread")?; + + Ok(Self { _watcher: watcher }) + } +} + +/// Coalesce event bursts: fire once the channel has been quiet for +/// `debounce`, but no later than `4 × debounce` after the burst began. +fn debounce_loop(rx: mpsc::Receiver<()>, debounce: Duration, on_change: impl Fn()) { + let max_wait = debounce * 4; + // Block until the first event of a burst; a closed channel ends the loop. + while rx.recv().is_ok() { + let deadline = Instant::now() + max_wait; + loop { + let remaining = deadline.saturating_duration_since(Instant::now()); + match rx.recv_timeout(debounce.min(remaining)) { + Ok(()) if Instant::now() < deadline => continue, + Ok(()) | Err(mpsc::RecvTimeoutError::Timeout) => break, + Err(mpsc::RecvTimeoutError::Disconnected) => return, + } + } + on_change(); + } +} + +/// The paths to watch for one repo root: the working directory, plus the +/// private and common git dirs when they live outside it (linked worktrees). +fn watch_targets(root: &Path) -> Result { + let repo = crate::GitRepository::open(root) + .with_context(|| format!("Failed to open git repository at {}", root.display()))?; + // Canonical paths: gix reports a linked worktree's common dir as + // `.git/worktrees//../..`, and event paths must prefix-match. + let canonical = |p: PathBuf| p.canonicalize().unwrap_or(p); + let workdir = canonical(repo.workdir().to_path_buf()); + let git_dir = canonical(repo.gitdir()); + let common_dir = canonical(repo.commondir()); + + let mut paths = vec![workdir.clone()]; + for dir in [&git_dir, &common_dir] { + if !dir.starts_with(&workdir) && !paths.contains(dir) { + paths.push(dir.clone()); + } + } + let mut git_dirs = vec![git_dir]; + if !git_dirs.contains(&common_dir) { + git_dirs.push(common_dir); + } + Ok(WatchTargets { paths, git_dirs }) +} + +/// True if an event at `path` cannot change what a review shows. Paths +/// outside any git dir are never noise. +fn is_noise(path: &Path, git_dirs: &[PathBuf]) -> bool { + match path_in_git_dir(path, git_dirs) { + Some(inner) => is_git_dir_noise(&inner), + None => false, + } +} + +/// If `path` is inside a git dir, return its path relative to that dir. Known +/// git dirs (which may lie outside the working directory) are checked first, +/// then any `.git` ancestor — which also covers nested repositories. +fn path_in_git_dir(path: &Path, git_dirs: &[PathBuf]) -> Option { + if let Some(inner) = git_dirs + .iter() + .filter_map(|dir| path.strip_prefix(dir).ok()) + .min_by_key(|inner| inner.components().count()) + { + return Some(inner.to_path_buf()); + } + path.ancestors() + .find(|a| a.file_name().is_some_and(|n| n == ".git")) + .and_then(|dot_git| path.strip_prefix(dot_git).ok()) + .map(Path::to_path_buf) +} + +/// Zed's filter for events inside a git dir (`inner` is relative to it). +/// An empty `inner` is the git dir itself, whose own metadata changes are +/// irrelevant. +fn is_git_dir_noise(inner: &Path) -> bool { + if inner.as_os_str().is_empty() { + return true; + } + let file_name = inner.file_name().and_then(|n| n.to_str()); + let extension = inner.extension().and_then(|e| e.to_str()); + + SKIPPED_FILE_NAMES_IN_DOT_GIT + .iter() + .any(|skipped| file_name == Some(skipped)) + || (inner.starts_with(LOGS_DIR) && inner != Path::new(LOGS_REF_STASH)) + || (inner.starts_with(INFO_DIR) && inner != Path::new(REPO_EXCLUDE)) + || SKIPPED_DIRS_IN_DOT_GIT + .iter() + .any(|skipped| inner.starts_with(skipped)) + || extension == Some("lock") + || (inner.components().count() == 1 && matches!(extension, Some("new") | Some("tmp"))) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::testutil::init_repo_with_commit; + use tempfile::TempDir; + + fn noise(inner: &str) -> bool { + is_git_dir_noise(Path::new(inner)) + } + + #[test] + fn git_dir_filter_drops_churn_and_keeps_state() { + // Dropped: the dir itself, objects, hooks, reflogs, locks, temp files. + assert!(noise("")); + assert!(noise("objects/ab/cdef")); + assert!(noise("objects/pack/pack-1.idx")); + assert!(noise("hooks/pre-commit")); + assert!(noise("lfs/objects/x")); + assert!(noise("fsmonitor--daemon/cookie")); + assert!(noise("rebase-merge/done")); + assert!(noise("rebase-apply/patch")); + assert!(noise("sequencer/todo")); + assert!(noise("logs/HEAD")); + assert!(noise("logs/refs/heads/main")); + assert!(noise("info/refs")); + assert!(noise("COMMIT_EDITMSG")); + assert!(noise("FETCH_HEAD")); + assert!(noise("ORIG_HEAD")); + assert!(noise("BISECT_LOG")); + assert!(noise("gc.pid")); + assert!(noise("index.lock")); + assert!(noise("refs/heads/main.lock")); + assert!(noise("index.tmp")); + assert!(noise("packed-refs.new")); + + // Kept: everything that feeds status and diffs. + assert!(!noise("index")); + assert!(!noise("HEAD")); + assert!(!noise("refs/heads/main")); + assert!(!noise("packed-refs")); + assert!(!noise("MERGE_HEAD")); + assert!(!noise("logs/refs/stash")); + assert!(!noise("info/exclude")); + assert!(!noise("config")); + // Temp-suffix rule only applies at the top level of the git dir. + assert!(!noise("refs/heads/feature.tmp")); + } + + #[test] + fn locates_git_dir_by_prefix_or_dot_git_ancestor() { + let linked = PathBuf::from("/main/.git/worktrees/wt"); + let git_dirs = vec![linked.clone(), PathBuf::from("/main/.git")]; + + // Explicit git dirs win, most specific first. + assert_eq!( + path_in_git_dir(&linked.join("index"), &git_dirs), + Some(PathBuf::from("index")) + ); + assert_eq!( + path_in_git_dir(Path::new("/main/.git/refs/heads/x"), &git_dirs), + Some(PathBuf::from("refs/heads/x")) + ); + // A nested repo's `.git` is found via the ancestor rule. + assert_eq!( + path_in_git_dir(Path::new("/wt/vendor/lib/.git/objects/aa"), &git_dirs), + Some(PathBuf::from("objects/aa")) + ); + // Regular working-tree files are outside any git dir. + assert_eq!( + path_in_git_dir(Path::new("/wt/src/lib.rs"), &git_dirs), + None + ); + assert!(!is_noise(Path::new("/wt/src/lib.rs"), &git_dirs)); + assert!(is_noise(Path::new("/main/.git/objects/aa/bb"), &git_dirs)); + } + + #[test] + fn watch_targets_cover_linked_worktree_git_dirs() { + let dir = TempDir::new().unwrap(); + init_repo_with_commit(dir.path()); + let main = dir.path().canonicalize().unwrap(); + + let targets = watch_targets(&main).unwrap(); + assert_eq!(targets.paths, vec![main.clone()]); + assert_eq!(targets.git_dirs, vec![main.join(".git")]); + + // A linked worktree keeps its index/HEAD in the main repo's `.git`. + let wt = dir.path().join("wt"); + let status = std::process::Command::new("git") + .args(["worktree", "add", "-b", "wt", wt.to_str().unwrap()]) + .current_dir(dir.path()) + .status() + .unwrap(); + assert!(status.success()); + let wt = wt.canonicalize().unwrap(); + + let targets = watch_targets(&wt).unwrap(); + let private = main.join(".git/worktrees/wt"); + assert_eq!(targets.paths, vec![wt, private.clone(), main.join(".git")]); + assert_eq!(targets.git_dirs, vec![private, main.join(".git")]); + } + + #[test] + fn reports_working_tree_writes_once_per_burst() { + let dir = TempDir::new().unwrap(); + init_repo_with_commit(dir.path()); + let root = dir.path().canonicalize().unwrap(); + + let (tx, rx) = mpsc::channel(); + let _watcher = ChangeWatcher::start( + std::slice::from_ref(&root), + Duration::from_millis(100), + move || { + let _ = tx.send(()); + }, + ) + .unwrap(); + // Let the OS-level watch settle before producing events. + std::thread::sleep(Duration::from_millis(300)); + + for i in 0..5 { + std::fs::write(root.join("a.txt"), format!("edit {i}\n")).unwrap(); + } + rx.recv_timeout(Duration::from_secs(5)) + .expect("a working-tree write must be reported"); + + // The burst is coalesced: after it settles, no further callbacks. + while rx.recv_timeout(Duration::from_millis(600)).is_ok() {} + + // Object churn inside `.git` is filtered out entirely. + std::fs::create_dir_all(root.join(".git/objects/ab")).unwrap(); + std::fs::write(root.join(".git/objects/ab/cdef"), b"blob").unwrap(); + assert!( + rx.recv_timeout(Duration::from_millis(800)).is_err(), + "object writes must not trigger a callback" + ); + + // But a ref update (e.g. a commit) is reported. + std::fs::write(root.join(".git/refs/heads/topic"), b"0000\n").unwrap(); + rx.recv_timeout(Duration::from_secs(5)) + .expect("a ref write must be reported"); + } +} diff --git a/crates/ui_gpui/src/main_screen/mod.rs b/crates/ui_gpui/src/main_screen/mod.rs index f1450650..8bd64208 100644 --- a/crates/ui_gpui/src/main_screen/mod.rs +++ b/crates/ui_gpui/src/main_screen/mod.rs @@ -226,6 +226,7 @@ pub struct MainScreen { _new_project_dialog_subscription: Option, _about_dialog_subscription: Option, _window_bounds_subscription: Subscription, + _window_activation_subscription: Subscription, } impl MainScreen { @@ -268,6 +269,8 @@ impl MainScreen { // Watch for window move / resize so we can persist bounds. let window_bounds_subscription = cx.observe_window_bounds(window, Self::on_window_bounds_changed); + let window_activation_subscription = + cx.observe_window_activation(window, Self::on_window_activation_changed); // Create the right (review) sidebar panel. let right_panel = cx.new(|cx| right_panel::RightPanel::new(window, cx)); @@ -311,6 +314,7 @@ impl MainScreen { _new_project_dialog_subscription: None, _about_dialog_subscription: None, _window_bounds_subscription: window_bounds_subscription, + _window_activation_subscription: window_activation_subscription, }; // Request initial chat session list @@ -344,13 +348,14 @@ impl MainScreen { self.ensure_sidebar_animation_task(cx); // When opening, make sure the panel reflects the current session and - // has fresh data. - if should_expand { - let session_id = self.current_session_id.clone(); - self.right_panel.update(cx, |panel, cx| { - panel.set_session(session_id, cx); - }); - } + // has fresh data. When closing, detach it so it stops watching and + // refreshing for nobody. + let session_id = should_expand + .then(|| self.current_session_id.clone()) + .flatten(); + self.right_panel.update(cx, |panel, cx| { + panel.set_session(session_id, cx); + }); // Persist the open/closed state for the active session. if let Some(session_id) = &self.current_session_id { @@ -555,6 +560,15 @@ impl MainScreen { crate::update_ui_settings(cx, f); } + /// Coming back to the window: re-list the review panel's changes. The + /// panel's filesystem watcher normally keeps it fresh; this covers the + /// cases a watcher can miss (e.g. inotify limits on large trees). + fn on_window_activation_changed(&mut self, window: &mut gpui::Window, cx: &mut Context) { + if window.is_window_active() && !self.right_sidebar_collapsed { + self.right_panel.update(cx, |panel, cx| panel.reload(cx)); + } + } + /// Called when the window is moved or resized. fn on_window_bounds_changed(&mut self, window: &mut gpui::Window, cx: &mut Context) { let bounds = window.bounds(); @@ -1285,11 +1299,12 @@ impl MainScreen { } self.right_sidebar_collapsed = !restored_open; - // Point the panel at the new session (clears stale tree/diff) and, when - // open, request fresh data. + // Point the panel at the new session when it is open (clears stale + // data and requests fresh); a closed panel stays detached until opened. self.right_panel_session_id = new_session_id.clone(); + let panel_session_id = restored_open.then(|| new_session_id.clone()).flatten(); self.right_panel.update(cx, |panel, cx| { - panel.set_session(new_session_id.clone(), cx); + panel.set_session(panel_session_id, cx); }); } } diff --git a/crates/ui_gpui/src/main_screen/right_panel/review_view.rs b/crates/ui_gpui/src/main_screen/right_panel/review_view.rs index 677cb6c4..0844a2d8 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/review_view.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/review_view.rs @@ -13,6 +13,13 @@ //! file without a diff is requested. Hunks (changed lines + a few context //! lines) are computed once on arrival and cached — rendering never diffs, and //! the element count scales with changed lines, not file sizes. +//! +//! Freshness: while the view has a listing it owns a [`git::ChangeWatcher`] +//! on the listed repos and re-requests the listing whenever the watcher +//! reports activity (the main screen also reloads on window activation as a +//! safety net). Each changed file carries a fingerprint; a cached diff whose +//! listing entry changed is stale and re-requested, but keeps rendering until +//! its replacement arrives, so nothing flickers. use crate::shared::file_icons; use crate::tool_cards::diff_card::{added_row_colors, deleted_row_colors, render_diff_hunks}; @@ -21,7 +28,7 @@ use code_assistant_core::session::{ReviewMode, ReviewScanState}; use git::{ChangeStatus, ChangedFile}; use gpui::{ AnimationExt, Context, Entity, EventEmitter, FocusHandle, Focusable, FontWeight, Render, - Subscription, Window, div, prelude::*, px, rems, + Subscription, Task, Window, div, prelude::*, px, rems, }; use gpui_component::{ ActiveTheme, Icon, Sizable, Size, @@ -132,6 +139,18 @@ type FileKey = (PathBuf, String); /// mismatches, whatever the global's current generation is. const GENERATION_UNSEEN: u64 = u64::MAX; +/// Quiet period the change watcher waits before reporting a burst of +/// filesystem events, so an edit (or a build) costs one scan. +const REVIEW_WATCH_DEBOUNCE: std::time::Duration = std::time::Duration::from_millis(300); + +/// A prepared diff together with the listing entry it was loaded for. When a +/// later listing carries a different entry for the same path (new +/// fingerprint or status), the diff is stale. +struct LoadedDiff { + file: ChangedFile, + prepared: PreparedReviewDiff, +} + pub struct ReviewView { session_id: Option, mode_state: Entity>>, @@ -149,13 +168,22 @@ pub struct ReviewView { /// Persisted default base ref, seeds a repo's base when it has no override. default_base: Option, - /// Prepared diffs by file, filled lazily one request at a time. - file_diffs: HashMap, + /// Prepared diffs by file, filled lazily one request at a time. A stale + /// entry (see [`LoadedDiff`]) stays here — and on screen — until its + /// replacement arrives. + file_diffs: HashMap, /// Files the user collapsed (default is expanded). collapsed_files: HashSet, - /// The single outstanding diff request; arrivals for anything else are - /// stale (e.g. from before a mode/base change) and dropped. - in_flight: Option, + /// The single outstanding diff request, with the listing entry it was + /// made for; arrivals for anything else are stale (e.g. from before a + /// mode/base change) and dropped. + in_flight: Option<(FileKey, ChangedFile)>, + + /// Filesystem watcher on the listed repos (keyed by their roots so a + /// changed set restarts it). Dropping it stops watching. + watcher: Option<(Vec, git::ChangeWatcher)>, + /// Forwards watcher callbacks to `request_listing` on the UI thread. + watch_task: Option>, /// Generation of the consumed listing. Change detection per frame is a /// plain integer compare against the global's generation — no clones. @@ -195,6 +223,8 @@ impl ReviewView { file_diffs: HashMap::new(), collapsed_files: HashSet::new(), in_flight: None, + watcher: None, + watch_task: None, listing_generation: GENERATION_UNSEEN, diff_generation: GENERATION_UNSEEN, focus_handle: cx.focus_handle(), @@ -214,6 +244,9 @@ impl ReviewView { self.file_diffs.clear(); self.collapsed_files.clear(); self.in_flight = None; + // A new session lists its own repos; the watcher follows the listing. + self.watcher = None; + self.watch_task = None; // Restore the persisted compare mode for this session. The selector // resyncs from the echoed listing on the next render. @@ -258,6 +291,48 @@ impl ReviewView { self.request_listing(cx); } + /// Keep a change watcher running on exactly the listed repos. Watcher + /// callbacks (background thread) are forwarded through a one-slot channel + /// to `request_listing` on the UI thread; the listing's fingerprints then + /// decide which diffs are stale. + fn ensure_watcher(&mut self, cx: &mut Context) { + let roots: Vec = self.repos.iter().map(|r| r.repo_root.clone()).collect(); + if roots.is_empty() { + self.watcher = None; + self.watch_task = None; + return; + } + if self.watcher.as_ref().is_some_and(|(r, _)| *r == roots) { + return; + } + + // A bounded(1) channel coalesces callbacks that land while the UI + // thread is still busy; dropping the watcher closes it, ending the task. + let (tx, rx) = async_channel::bounded::<()>(1); + match git::ChangeWatcher::start(&roots, REVIEW_WATCH_DEBOUNCE, move || { + let _ = tx.try_send(()); + }) { + Ok(watcher) => { + self.watcher = Some((roots, watcher)); + self.watch_task = Some(cx.spawn(async move |this, cx| { + while rx.recv().await.is_ok() { + if this + .update(cx, |this, cx| this.request_listing(cx)) + .is_err() + { + break; + } + } + })); + } + Err(e) => { + tracing::warn!("Review panel: change watcher unavailable: {e:#}"); + self.watcher = None; + self.watch_task = None; + } + } + } + fn request_listing(&self, cx: &mut Context) { let Some(session_id) = self.session_id.clone() else { return; @@ -285,7 +360,14 @@ impl ReviewView { } for file in §ion.files { let key = (section.repo_root.clone(), file.path.clone()); - if self.collapsed_files.contains(&key) || self.file_diffs.contains_key(&key) { + // A diff loaded for exactly this listing entry is current; + // one loaded for an older entry (fingerprint moved) is stale + // and gets requested again. + let is_current = self + .file_diffs + .get(&key) + .is_some_and(|loaded| &loaded.file == file); + if self.collapsed_files.contains(&key) || is_current { continue; } next = Some(( @@ -298,7 +380,7 @@ impl ReviewView { } if let Some((repo_root, base, file)) = next { - self.in_flight = Some((repo_root.clone(), file.path.clone())); + self.in_flight = Some(((repo_root.clone(), file.path.clone()), file.clone())); if let Some(gpui) = cx.try_global::() { gpui.cmd_get_review_file_diff(session_id, repo_root, self.mode, base, file); } @@ -396,7 +478,7 @@ impl ReviewView { .collect(); self.file_diffs.retain(|key, _| live.contains(key)); self.collapsed_files.retain(|key| live.contains(key)); - if let Some(in_flight) = &self.in_flight + if let Some((in_flight, _)) = &self.in_flight && !live.contains(in_flight) { self.in_flight = None; @@ -424,6 +506,7 @@ impl ReviewView { } } + self.ensure_watcher(cx); self.ensure_diff_request(cx); } @@ -441,9 +524,14 @@ impl ReviewView { if let Some(d) = diff { let key = (d.repo_root, d.path); - if self.in_flight.as_ref() == Some(&key) { - self.in_flight = None; - self.file_diffs.insert(key, d.prepared); + if let Some((_, file)) = self.in_flight.take_if(|(k, _)| *k == key) { + self.file_diffs.insert( + key, + LoadedDiff { + file, + prepared: d.prepared, + }, + ); } } self.ensure_diff_request(cx); @@ -634,8 +722,8 @@ impl ReviewView { let key: FileKey = (repo_root.to_path_buf(), file.path.clone()); let collapsed = self.collapsed_files.contains(&key); - let entry = self.file_diffs.get(&key); - let loading = self.in_flight.as_ref() == Some(&key); + let entry = self.file_diffs.get(&key).map(|loaded| &loaded.prepared); + let loading = self.in_flight.as_ref().is_some_and(|(k, _)| *k == key); // Right-hand slot of the file header. let indicator: gpui::AnyElement = match entry { diff --git a/crates/ui_gpui/src/shared/review_cache.rs b/crates/ui_gpui/src/shared/review_cache.rs index d5f05088..896ec9d2 100644 --- a/crates/ui_gpui/src/shared/review_cache.rs +++ b/crates/ui_gpui/src/shared/review_cache.rs @@ -111,6 +111,7 @@ mod tests { path: "src/lib.rs".into(), orig_path: None, status: git::ChangeStatus::Modified, + fingerprint: Some("abc:def".into()), }], stats: git::DiffStats { additions: 12, diff --git a/crates/ui_gpui/src/tool_cards/diff_card.rs b/crates/ui_gpui/src/tool_cards/diff_card.rs index 1376b93e..99dcf33a 100644 --- a/crates/ui_gpui/src/tool_cards/diff_card.rs +++ b/crates/ui_gpui/src/tool_cards/diff_card.rs @@ -548,9 +548,43 @@ pub struct DiffLine { pub emphasis: Vec>, } -/// Replace blocks larger than this skip the word-level diff — pairing lines -/// across big rewrites produces noise, not signal (Zed caps similarly). -const MAX_WORD_DIFF_LINES: usize = 16; +/// Replace blocks with more lines than this (per side) skip the word-level +/// diff — pairing words across big rewrites produces noise, not signal, and +/// the word diff's cost grows with the block. Same cap as Zed. +const MAX_WORD_DIFF_LINES: usize = 8; + +/// A line whose emphasized share of non-whitespace bytes exceeds this is +/// mostly rewritten: word emphasis would light up most of it, so the line is +/// shown as a plain change instead. Long prose paragraphs share enough +/// common words ("the", "data", …) to pass `similar`'s similarity cutoff +/// while every other word changed; this is what filters that out. +const MAX_EMPHASIS_SHARE: f32 = 0.5; + +/// Merge emphasis ranges whose gap is whitespace only: word tokens on either +/// side of an unchanged space are one change to the eye. +fn merge_whitespace_gaps(emphasis: &mut Vec>, text: &str) { + emphasis.dedup_by(|next, prev| { + let gap = &text[prev.end..next.start]; + if gap.chars().all(char::is_whitespace) { + prev.end = next.end; + true + } else { + false + } + }); +} + +/// True if emphasizing `emphasis` would cover more than [`MAX_EMPHASIS_SHARE`] +/// of the line's non-whitespace bytes. +fn emphasis_is_noise(emphasis: &[std::ops::Range], text: &str) -> bool { + let non_ws = |s: &str| s.bytes().filter(|b| !b.is_ascii_whitespace()).count(); + let total = non_ws(text); + if total == 0 { + return false; + } + let emphasized: usize = emphasis.iter().map(|r| non_ws(&text[r.clone()])).sum(); + emphasized as f32 / total as f32 > MAX_EMPHASIS_SHARE +} /// Expand one diff op into [`DiffLine`]s, with word-level emphasis for small /// replace blocks. `iter_inline_changes` falls back to plain changes on its @@ -578,6 +612,10 @@ fn collect_change_lines<'a>( r.end = r.end.min(trimmed_len); r.start < r.end }); + merge_whitespace_gaps(&mut emphasis, &text); + if emphasis_is_noise(&emphasis, &text) { + emphasis.clear(); + } out.push(DiffLine { tag: change.tag(), text: text.into(), @@ -1195,6 +1233,50 @@ mod tests { ); } + #[test] + fn word_diff_merges_ranges_split_only_by_whitespace() { + let lines = compute_diff_lines("keep foo bar keep\n", "keep qux quux keep\n"); + let ins = lines.iter().find(|l| l.tag == ChangeTag::Insert).unwrap(); + // "qux" and "quux" are separate word tokens with an unchanged space + // between them; visually that is one change. + assert_eq!(ins.emphasis.len(), 1); + assert_eq!(&ins.text[ins.emphasis[0].clone()], "qux quux"); + } + + #[test] + fn word_diff_kept_for_small_edit_in_long_paragraph() { + // A long prose paragraph (one line) with a single changed word is + // exactly where word emphasis helps most — length must not disable it. + let filler = "the data center deployment ".repeat(25); + let old = format!("{filler}serving a jurisdiction.\n"); + let new = format!("{filler}serving one or more jurisdictions.\n"); + assert!(old.len() > 512); + let lines = compute_diff_lines(&old, &new); + let ins = lines.iter().find(|l| l.tag == ChangeTag::Insert).unwrap(); + assert_eq!(ins.emphasis.len(), 1); + assert_eq!( + &ins.text[ins.emphasis[0].clone()], + "one or more jurisdictions" + ); + } + + #[test] + fn word_diff_dropped_when_most_of_the_line_changed() { + // Enough tokens (spaces, one word) match for `similar` to attempt a + // word diff, but nearly every word changed: emphasizing most of the + // line is noise, so the line is shown as a plain change instead. + let lines = compute_diff_lines( + "one two three four five six seven\n", + "uno dos tres four cinco seis siete\n", + ); + assert!( + lines + .iter() + .filter(|l| l.tag != ChangeTag::Equal) + .all(|l| l.emphasis.is_empty()) + ); + } + #[test] fn single_sided_hunk_is_one_pure_hunk() { let hunks = single_sided_hunk("a\nb\nc\n", ChangeTag::Insert); diff --git a/docs/gpui-reference.md b/docs/gpui-reference.md new file mode 100644 index 00000000..43a621e9 --- /dev/null +++ b/docs/gpui-reference.md @@ -0,0 +1,113 @@ +# GPUI reference + +Adapted from Zed's `.rules` file. The "Recent API changes" section is +mirrored in `AGENTS.md`. + +GPUI is a UI framework which also provides primitives for state and concurrency management. + +## Context + +Context types allow interaction with global state, windows, entities, and system services. They are typically passed to functions as the argument named `cx`. When a function takes callbacks they come after the `cx` parameter. + +* `App` is the root context type, providing access to global state and read and update of entities. +* `Context` is provided when updating an `Entity`. This context dereferences into `App`, so functions which take `&App` can also take `&Context`. +* `AsyncApp` and `AsyncWindowContext` are provided by `cx.spawn` and `cx.spawn_in`. These can be held across await points. + +## `Window` + +`Window` provides access to the state of an application window. It is passed to functions as an argument named `window` and comes before `cx` when present. It is used for managing focus, dispatching actions, directly drawing, getting user input state, etc. + +## Entities + +An `Entity` is a handle to state of type `T`. With `thing: Entity`: + +* `thing.entity_id()` returns `EntityId` +* `thing.downgrade()` returns `WeakEntity` +* `thing.read(cx: &App)` returns `&T`. +* `thing.read_with(cx, |thing: &T, cx: &App| ...)` returns the closure's return value. +* `thing.update(cx, |thing: &mut T, cx: &mut Context| ...)` allows the closure to mutate the state, and provides a `Context` for interacting with the entity. It returns the closure's return value. +* `thing.update_in(cx, |thing: &mut T, window: &mut Window, cx: &mut Context| ...)` takes a `AsyncWindowContext` or `VisualTestContext`. It's the same as `update` while also providing the `Window`. + +Within the closures, the inner `cx` provided to the closure must be used instead of the outer `cx` to avoid issues with multiple borrows. + +Trying to update an entity while it's already being updated must be avoided as this will cause a panic. + +When `read_with`, `update`, or `update_in` are used with an async context, the closure's return value is wrapped in an `anyhow::Result`. + +`WeakEntity` is a weak handle. It has `read_with`, `update`, and `update_in` methods that work the same, but always return an `anyhow::Result` so that they can fail if the entity no longer exists. This can be useful to avoid memory leaks - if entities have mutually recursive handles to each other they will never be dropped. + +## Concurrency + +All use of entities and UI rendering occurs on a single foreground thread. + +`cx.spawn(async move |cx| ...)` runs an async closure on the foreground thread. Within the closure, `cx` is an async context like `AsyncApp` or `AsyncWindowContext`. + +When the outer cx is a `Context`, the use of `spawn` instead looks like `cx.spawn(async move |handle, cx| ...)`, where `handle: WeakEntity`. + +To do work on other threads, `cx.background_spawn(async move { ... })` is used. Often this background task is awaited on by a foreground task which uses the results to update state. + +Both `cx.spawn` and `cx.background_spawn` return a `Task`, which is a future that can be awaited upon. If this task is dropped, then its work is cancelled. To prevent this one of the following must be done: + +* Awaiting the task in some other async context. +* Detaching the task via `task.detach()` or `task.detach_and_log_err(cx)`, allowing it to run indefinitely. +* Storing the task in a field, if the work should be halted when the struct is dropped. + +A task which doesn't do anything but provide a value can be created with `Task::ready(value)`. + +## Elements + +The `Render` trait is used to render some state into an element tree that is laid out using flexbox layout. An `Entity` where `T` implements `Render` is sometimes called a "view". + +Example: + +``` +struct TextWithBorder(SharedString); + +impl Render for TextWithBorder { + fn render(&mut self, _window: &mut Window, _cx: &mut Context) -> impl IntoElement { + div().border_1().child(self.0.clone()) + } +} +``` + +Since `impl IntoElement for SharedString` exists, it can be used as an argument to `child`. `SharedString` is used to avoid copying strings, and is either an `&'static str` or `Arc`. + +UI components that are constructed just to be turned into elements can instead implement the `RenderOnce` trait, which is similar to `Render`, but its `render` method takes ownership of `self`. Types that implement this trait can use `#[derive(IntoElement)]` to use them directly as children. + +The style methods on elements are similar to those used by Tailwind CSS. + +If some attributes or children of an element tree are conditional, `.when(condition, |this| ...)` can be used to run the closure only when `condition` is true. Similarly, `.when_some(option, |this, value| ...)` runs the closure when the `Option` has a value. + +## Input events + +Input event handlers can be registered on an element via methods like `.on_click(|event, window, cx: &mut App| ...)`. + +Often event handlers will want to update the entity that's in the current `Context`. The `cx.listener` method provides this - its use looks like `.on_click(cx.listener(|this: &mut T, event, window, cx: &mut Context| ...)`. + +## Actions + +Actions are dispatched via user keyboard interaction or in code via `window.dispatch_action(SomeAction.boxed_clone(), cx)` or `focus_handle.dispatch_action(&SomeAction, window, cx)`. + +Actions with no data defined with the `actions!(some_namespace, [SomeAction, AnotherAction])` macro call. Otherwise the `Action` derive macro is used. Doc comments on actions are displayed to the user. + +Action handlers can be registered on an element via the event handler `.on_action(|action, window, cx| ...)`. Like other event handlers, this is often used with `cx.listener`. + +## Notify + +When a view's state has changed in a way that may affect its rendering, it should call `cx.notify()`. This will cause the view to be rerendered. It will also cause any observe callbacks registered for the entity with `cx.observe` to be called. + +## Entity events + +While updating an entity (`cx: Context`), it can emit an event using `cx.emit(event)`. Entities register which events they can emit by declaring `impl EventEmittor for EntityType {}`. + +Other entities can then register a callback to handle these events by doing `cx.subscribe(other_entity, |this, other_entity, event, cx| ...)`. This will return a `Subscription` which deregisters the callback when dropped. Typically `cx.subscribe` happens when creating a new entity and the subscriptions are stored in a `_subscriptions: Vec` field. + +## Recent API changes + +GPUI has had some changes to its APIs. Always write code using the new APIs: + +* `spawn` methods now take async closures (`AsyncFn`), and so should be called like `cx.spawn(async move |cx| ...)`. +* Use `Entity`. This replaces `Model` and `View` which no longer exist and should NEVER be used. +* Use `App` references. This replaces `AppContext` which no longer exists and should NEVER be used. +* Use `Context` references. This replaces `ModelContext` which no longer exists and should NEVER be used. +* `Window` is now passed around explicitly. The new interface adds a `Window` reference parameter to some methods, and adds some new "*_in" methods for plumbing `Window`. The old types `WindowContext` and `ViewContext` should NEVER be used.