From bda3dbf6cc12b2fa754348439b737bc6c5ac485c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stephan=20A=C3=9Fmus?= Date: Mon, 7 Sep 2026 08:28:17 +0200 Subject: [PATCH 1/4] docs: condense AGENTS.md, move GPUI reference to docs/gpui-reference.md Trim MCP client, permission tiers and UI communication sections to the essentials and point to the existing docs. Drop sections duplicated by the crate layer diagram and key entry points. Move the GPUI API primer (copied from Zed's .rules) into docs/gpui-reference.md, keeping only the deprecated-API rules in AGENTS.md. --- AGENTS.md | 244 +++++++---------------------------------- docs/gpui-reference.md | 113 +++++++++++++++++++ 2 files changed, 151 insertions(+), 206 deletions(-) create mode 100644 docs/gpui-reference.md 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/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. From 747bc9a2565f0425dce3ee026931cbf7421fd0d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stephan=20A=C3=9Fmus?= Date: Mon, 7 Sep 2026 08:39:49 +0200 Subject: [PATCH 2/4] fix(ui_gpui): keep review diffs fresh while the agent edits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review panel only listed changes when it was opened or the session switched, and a prepared diff for a file still in the listing was never re-requested — so diffs went stale as soon as the agent kept editing. - git: ChangedFile gains a fingerprint (blob ids in branch mode via `diff --raw`; HEAD + mtime/size of the working-tree file otherwise) - ui_gpui: Gpui::files_changed_generation is bumped after every finished tool and at the end of a turn; the ReviewView re-lists (debounced) and re-requests only diffs whose listing entry changed, keeping the old hunks on screen until the replacement arrives --- crates/git/src/diff.rs | 163 ++++++++++++++++-- crates/ui_gpui/src/app/event_loop.rs | 7 + crates/ui_gpui/src/lib.rs | 18 ++ .../main_screen/right_panel/review_view.rs | 100 +++++++++-- crates/ui_gpui/src/shared/review_cache.rs | 1 + 5 files changed, 255 insertions(+), 34 deletions(-) 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/ui_gpui/src/app/event_loop.rs b/crates/ui_gpui/src/app/event_loop.rs index 84502d2d..c8cf4017 100644 --- a/crates/ui_gpui/src/app/event_loop.rs +++ b/crates/ui_gpui/src/app/event_loop.rs @@ -175,6 +175,10 @@ impl Gpui { message_container.end_tool_use(&id, cx); }); self.auto_scroll_if_following(cx); + // Any tool may have touched the working tree (edits, but also + // shell commands): let the Review panel re-list changes. + self.bump_files_changed_generation(); + cx.refresh(); } UiEvent::HiddenToolCompleted => { // Mark that a hidden tool completed - message container handles paragraph breaks @@ -505,6 +509,9 @@ impl Gpui { message.finish_any_thinking_blocks(cx); }); } + // Catch-all for the turn's file changes (see `EndTool`). + self.bump_files_changed_generation(); + cx.refresh(); } UiEvent::RollbackStreaming { id } => { // Discard all blocks produced by the failed request so the retry diff --git a/crates/ui_gpui/src/lib.rs b/crates/ui_gpui/src/lib.rs index 4ef33e7d..7b603a29 100644 --- a/crates/ui_gpui/src/lib.rs +++ b/crates/ui_gpui/src/lib.rs @@ -279,6 +279,11 @@ pub struct Gpui { /// Components compare their locally cached generation with this to know when to reload. config_generation: Arc, + /// Incremented whenever the viewed session may have changed files on disk + /// (a tool finished, a turn ended). The Review panel compares this with + /// its cached value and re-lists changes when it moved. + files_changed_generation: Arc, + /// Skills available to the current session, cached for the `/skill` /// input-area completion and submit-time invocation. Refreshed on /// session load via [`Gpui::refresh_skills`]. @@ -608,6 +613,7 @@ impl Gpui { )), config_generation: Arc::new(std::sync::atomic::AtomicU64::new(0)), + files_changed_generation: Arc::new(std::sync::atomic::AtomicU64::new(0)), skills: Arc::new(Mutex::new(Vec::new())), } @@ -869,6 +875,18 @@ impl Gpui { .load(std::sync::atomic::Ordering::Relaxed) } + /// Current files-changed generation; see the field docs. + pub fn files_changed_generation(&self) -> u64 { + self.files_changed_generation + .load(std::sync::atomic::Ordering::Relaxed) + } + + /// Note that the viewed session's files may have changed on disk. + pub fn bump_files_changed_generation(&self) { + self.files_changed_generation + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); + } + pub fn get_current_error(&self) -> Option { self.current_error.lock().unwrap().clone() } 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..d28f8435 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,12 @@ //! 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: the listing is re-requested (debounced) whenever the app bumps +//! [`Gpui::files_changed_generation`] — after every finished tool and at the +//! end of a turn. 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 +27,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 +138,18 @@ type FileKey = (PathBuf, String); /// mismatches, whatever the global's current generation is. const GENERATION_UNSEEN: u64 = u64::MAX; +/// Quiet period after a files-changed signal before the listing is +/// re-requested, so a burst of tool completions costs one scan. +const FILES_CHANGED_DEBOUNCE: std::time::Duration = std::time::Duration::from_millis(250); + +/// 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 +167,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)>, + + /// Last consumed [`Gpui::files_changed_generation`]; a newer value + /// schedules a debounced re-listing. + files_changed_seen: u64, + /// The pending debounced re-listing, if any. Dropping it cancels. + refresh_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 +222,8 @@ impl ReviewView { file_diffs: HashMap::new(), collapsed_files: HashSet::new(), in_flight: None, + files_changed_seen: Self::files_changed_generation(cx), + refresh_task: None, listing_generation: GENERATION_UNSEEN, diff_generation: GENERATION_UNSEEN, focus_handle: cx.focus_handle(), @@ -214,6 +243,9 @@ impl ReviewView { self.file_diffs.clear(); self.collapsed_files.clear(); self.in_flight = None; + // The listing requested below is fresh; earlier signals are moot. + self.refresh_task = None; + self.files_changed_seen = Self::files_changed_generation(cx); // Restore the persisted compare mode for this session. The selector // resyncs from the echoed listing on the next render. @@ -258,6 +290,31 @@ impl ReviewView { self.request_listing(cx); } + fn files_changed_generation(cx: &Context) -> u64 { + cx.try_global::() + .map_or(0, |g| g.files_changed_generation()) + } + + /// Re-list changes (debounced) when the app signals that the session's + /// files may have changed. The listing's fingerprints then decide which + /// diffs are stale; the rest keep their prepared hunks. + fn sync_files_changed(&mut self, cx: &mut Context) { + let generation = Self::files_changed_generation(cx); + if generation == self.files_changed_seen { + return; + } + self.files_changed_seen = generation; + if self.session_id.is_none() { + return; + } + // Replacing the task drops (cancels) a still-pending one, so a burst + // of signals ends in a single request. + self.refresh_task = Some(cx.spawn(async move |this, cx| { + cx.background_executor().timer(FILES_CHANGED_DEBOUNCE).await; + let _ = this.update(cx, |this, cx| this.request_listing(cx)); + })); + } + fn request_listing(&self, cx: &mut Context) { let Some(session_id) = self.session_id.clone() else { return; @@ -285,7 +342,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 +362,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 +460,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; @@ -441,9 +505,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 +703,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 { @@ -897,8 +966,9 @@ impl Focusable for ReviewView { impl Render for ReviewView { fn render(&mut self, window: &mut Window, cx: &mut Context) -> impl IntoElement { - // Pull fresh backend data before laying out. Both syncs are a cheap + // Pull fresh backend data before laying out. All syncs are a cheap // generation compare when nothing changed. + self.sync_files_changed(cx); self.sync_listing(window, cx); self.sync_diff(cx); 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, From 8b26ffe9b60a4fd96f1507ad9b10437bcbbf799f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stephan=20A=C3=9Fmus?= Date: Mon, 7 Sep 2026 09:16:54 +0200 Subject: [PATCH 3/4] feat(ui_gpui): refresh the review panel from a filesystem watcher Replace the tool-end trigger with git::ChangeWatcher: a recursive notify watch on each listed repo (plus the private and common git dirs of linked worktrees), debounced, with Zed's .git filter so object, hook, reflog, lock and temp-file churn never triggers a rescan. The ReviewView owns the watcher while it has a listing; closing the panel or switching sessions detaches it. Window activation reloads as a safety net for what a watcher can miss. --- Cargo.lock | 1 + crates/git/Cargo.toml | 1 + crates/git/src/lib.rs | 2 + crates/git/src/repository.rs | 6 + crates/git/src/watch.rs | 347 ++++++++++++++++++ crates/ui_gpui/src/app/event_loop.rs | 7 - crates/ui_gpui/src/lib.rs | 18 - crates/ui_gpui/src/main_screen/mod.rs | 35 +- .../main_screen/right_panel/review_view.rs | 92 +++-- 9 files changed, 437 insertions(+), 72 deletions(-) create mode 100644 crates/git/src/watch.rs 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/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/app/event_loop.rs b/crates/ui_gpui/src/app/event_loop.rs index c8cf4017..84502d2d 100644 --- a/crates/ui_gpui/src/app/event_loop.rs +++ b/crates/ui_gpui/src/app/event_loop.rs @@ -175,10 +175,6 @@ impl Gpui { message_container.end_tool_use(&id, cx); }); self.auto_scroll_if_following(cx); - // Any tool may have touched the working tree (edits, but also - // shell commands): let the Review panel re-list changes. - self.bump_files_changed_generation(); - cx.refresh(); } UiEvent::HiddenToolCompleted => { // Mark that a hidden tool completed - message container handles paragraph breaks @@ -509,9 +505,6 @@ impl Gpui { message.finish_any_thinking_blocks(cx); }); } - // Catch-all for the turn's file changes (see `EndTool`). - self.bump_files_changed_generation(); - cx.refresh(); } UiEvent::RollbackStreaming { id } => { // Discard all blocks produced by the failed request so the retry diff --git a/crates/ui_gpui/src/lib.rs b/crates/ui_gpui/src/lib.rs index 7b603a29..4ef33e7d 100644 --- a/crates/ui_gpui/src/lib.rs +++ b/crates/ui_gpui/src/lib.rs @@ -279,11 +279,6 @@ pub struct Gpui { /// Components compare their locally cached generation with this to know when to reload. config_generation: Arc, - /// Incremented whenever the viewed session may have changed files on disk - /// (a tool finished, a turn ended). The Review panel compares this with - /// its cached value and re-lists changes when it moved. - files_changed_generation: Arc, - /// Skills available to the current session, cached for the `/skill` /// input-area completion and submit-time invocation. Refreshed on /// session load via [`Gpui::refresh_skills`]. @@ -613,7 +608,6 @@ impl Gpui { )), config_generation: Arc::new(std::sync::atomic::AtomicU64::new(0)), - files_changed_generation: Arc::new(std::sync::atomic::AtomicU64::new(0)), skills: Arc::new(Mutex::new(Vec::new())), } @@ -875,18 +869,6 @@ impl Gpui { .load(std::sync::atomic::Ordering::Relaxed) } - /// Current files-changed generation; see the field docs. - pub fn files_changed_generation(&self) -> u64 { - self.files_changed_generation - .load(std::sync::atomic::Ordering::Relaxed) - } - - /// Note that the viewed session's files may have changed on disk. - pub fn bump_files_changed_generation(&self) { - self.files_changed_generation - .fetch_add(1, std::sync::atomic::Ordering::Relaxed); - } - pub fn get_current_error(&self) -> Option { self.current_error.lock().unwrap().clone() } 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 d28f8435..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 @@ -14,9 +14,10 @@ //! lines) are computed once on arrival and cached — rendering never diffs, and //! the element count scales with changed lines, not file sizes. //! -//! Freshness: the listing is re-requested (debounced) whenever the app bumps -//! [`Gpui::files_changed_generation`] — after every finished tool and at the -//! end of a turn. Each changed file carries a fingerprint; a cached diff whose +//! 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. @@ -138,9 +139,9 @@ type FileKey = (PathBuf, String); /// mismatches, whatever the global's current generation is. const GENERATION_UNSEEN: u64 = u64::MAX; -/// Quiet period after a files-changed signal before the listing is -/// re-requested, so a burst of tool completions costs one scan. -const FILES_CHANGED_DEBOUNCE: std::time::Duration = std::time::Duration::from_millis(250); +/// 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 @@ -178,11 +179,11 @@ pub struct ReviewView { /// mode/base change) and dropped. in_flight: Option<(FileKey, ChangedFile)>, - /// Last consumed [`Gpui::files_changed_generation`]; a newer value - /// schedules a debounced re-listing. - files_changed_seen: u64, - /// The pending debounced re-listing, if any. Dropping it cancels. - refresh_task: Option>, + /// 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. @@ -222,8 +223,8 @@ impl ReviewView { file_diffs: HashMap::new(), collapsed_files: HashSet::new(), in_flight: None, - files_changed_seen: Self::files_changed_generation(cx), - refresh_task: None, + watcher: None, + watch_task: None, listing_generation: GENERATION_UNSEEN, diff_generation: GENERATION_UNSEEN, focus_handle: cx.focus_handle(), @@ -243,9 +244,9 @@ impl ReviewView { self.file_diffs.clear(); self.collapsed_files.clear(); self.in_flight = None; - // The listing requested below is fresh; earlier signals are moot. - self.refresh_task = None; - self.files_changed_seen = Self::files_changed_generation(cx); + // 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. @@ -290,29 +291,46 @@ impl ReviewView { self.request_listing(cx); } - fn files_changed_generation(cx: &Context) -> u64 { - cx.try_global::() - .map_or(0, |g| g.files_changed_generation()) - } - - /// Re-list changes (debounced) when the app signals that the session's - /// files may have changed. The listing's fingerprints then decide which - /// diffs are stale; the rest keep their prepared hunks. - fn sync_files_changed(&mut self, cx: &mut Context) { - let generation = Self::files_changed_generation(cx); - if generation == self.files_changed_seen { + /// 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; } - self.files_changed_seen = generation; - if self.session_id.is_none() { + if self.watcher.as_ref().is_some_and(|(r, _)| *r == roots) { return; } - // Replacing the task drops (cancels) a still-pending one, so a burst - // of signals ends in a single request. - self.refresh_task = Some(cx.spawn(async move |this, cx| { - cx.background_executor().timer(FILES_CHANGED_DEBOUNCE).await; - let _ = this.update(cx, |this, cx| this.request_listing(cx)); - })); + + // 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) { @@ -488,6 +506,7 @@ impl ReviewView { } } + self.ensure_watcher(cx); self.ensure_diff_request(cx); } @@ -966,9 +985,8 @@ impl Focusable for ReviewView { impl Render for ReviewView { fn render(&mut self, window: &mut Window, cx: &mut Context) -> impl IntoElement { - // Pull fresh backend data before laying out. All syncs are a cheap + // Pull fresh backend data before laying out. Both syncs are a cheap // generation compare when nothing changed. - self.sync_files_changed(cx); self.sync_listing(window, cx); self.sync_diff(cx); From 46003f91a220a7998dac0d3a0639b6728e64321a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stephan=20A=C3=9Fmus?= Date: Mon, 7 Sep 2026 09:27:07 +0200 Subject: [PATCH 4/4] fix(ui_gpui): quieter word-level diff emphasis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes to the intra-line word diff: - Drop a line's emphasis when it would cover more than half of the line's non-whitespace bytes. Rewritten prose paragraphs share enough common words to pass similar's ratio cutoff, yet the result lit up every other word and stray spaces; a mostly rewritten line now shows as a plain change. A small edit inside a long paragraph keeps its emphasis — length is not the criterion. - Merge emphasis ranges separated only by whitespace, so two changed words with an unchanged space between them highlight as one. The line cap follows Zed (8 per side). --- crates/ui_gpui/src/tool_cards/diff_card.rs | 88 +++++++++++++++++++++- 1 file changed, 85 insertions(+), 3 deletions(-) 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);