diff --git a/crates/workspace/src/views/repo/files.rs b/crates/workspace/src/views/repo/files.rs index 1ff85c0..ebd3fe9 100644 --- a/crates/workspace/src/views/repo/files.rs +++ b/crates/workspace/src/views/repo/files.rs @@ -59,7 +59,6 @@ pub(super) struct RepoFilesView { commits: HashMap, pending_commits: Vec, loading_commits: bool, - generation: u64, tasks: Vec>>, } @@ -80,7 +79,6 @@ impl RepoFilesView { commits: HashMap::new(), pending_commits: Vec::new(), loading_commits: false, - generation: 0, tasks: Vec::new(), } } @@ -142,7 +140,6 @@ impl RepoFilesView { self.md = None; self.code = None; self.readme_name = None; - self.generation += 1; } /// Refresh after the mirror caught up with the remote. @@ -466,7 +463,6 @@ impl RepoFilesView { let path = path.to_string(); self.load_commit(&path, cx); - let generation = self.generation; let task: Task> = cx.spawn_in(window, async move |this, cx| { let path_for_read = path.clone(); @@ -496,11 +492,6 @@ impl RepoFilesView { .await; this.update_in(cx, |this, window, cx| { - if generation != this.generation { - this.loading_files.remove(&path); - return; - } - this.loading_files.remove(&path); match content { @@ -616,7 +607,6 @@ impl RepoFilesView { self.loading_commits = true; let paths = std::mem::take(&mut self.pending_commits); - let generation = self.generation; let task: Task> = cx.spawn(async move |this, cx| { let rels: Vec = paths.iter().map(PathBuf::from).collect(); @@ -629,9 +619,7 @@ impl RepoFilesView { this.update(cx, |this, cx| { this.loading_commits = false; - if generation == this.generation - && let Ok(found) = result - { + if let Ok(found) = result { for (path, commit) in found { this.commits .insert(path.to_string_lossy().into_owned(), commit); diff --git a/crates/workspace/src/views/repo/history.rs b/crates/workspace/src/views/repo/history.rs index 9e47eb9..4ec8db5 100644 --- a/crates/workspace/src/views/repo/history.rs +++ b/crates/workspace/src/views/repo/history.rs @@ -23,8 +23,6 @@ pub(super) struct RepoHistoryView { loading_all_commits: bool, scroll_handle: VirtualListScrollHandle, item_sizes: Rc>>, - /// Bumped on reload, so an in-flight walk of the previous HEAD is discarded. - generation: u64, tasks: Vec>>, } @@ -38,7 +36,6 @@ impl RepoHistoryView { loading_all_commits: false, scroll_handle: VirtualListScrollHandle::new(), item_sizes: Rc::new(Vec::new()), - generation: 0, tasks: Vec::new(), } } @@ -54,7 +51,6 @@ impl RepoHistoryView { /// Drop the current list and walk HEAD again. pub(super) fn reload(&mut self, cx: &mut Context) { - self.generation += 1; self.all_commits = None; self.loading_all_commits = false; self.load(cx); @@ -70,7 +66,6 @@ impl RepoHistoryView { }; self.loading_all_commits = true; - let generation = self.generation; let task: Task> = cx.spawn(async move |this, cx| { let result = cx @@ -78,10 +73,6 @@ impl RepoHistoryView { .await; this.update(cx, |this, cx| { - if generation != this.generation { - return; - } - if let Ok(list) = result { let count = list.commits.len(); this.item_sizes = Rc::new(vec![size(px(0.), px(COMMIT_ROW_HEIGHT)); count]); diff --git a/crates/workspace/src/views/repo/mod.rs b/crates/workspace/src/views/repo/mod.rs index 6673c54..eacdae1 100644 --- a/crates/workspace/src/views/repo/mod.rs +++ b/crates/workspace/src/views/repo/mod.rs @@ -91,7 +91,6 @@ pub struct RepoDetailView { error: Option, head_commit: Option, refs: RefSwitcher, - ref_generation: u64, banners: Banners, tasks: Vec>>, _subscriptions: Vec, @@ -191,7 +190,6 @@ impl RepoDetailView { error: None, head_commit: None, refs, - ref_generation: 0, tasks: Vec::new(), banners: Banners::default(), focus_handle: cx.focus_handle(), @@ -342,16 +340,13 @@ impl RepoDetailView { let Some(announcement) = announcement else { return; }; + self.repo_started = true; let cache = GitStore::global(cx).cache().clone(); let addr = announcement.addr(); let clone_urls: Vec = announcement.clone.clone(); - // Captured before the loads start. - // A branch/tag switch bumps the generation, discarding the refresh below. - let refresh_generation = self.ref_generation; - let disk = { let cache = cache.clone(); let addr = addr.clone(); @@ -363,7 +358,7 @@ impl RepoDetailView { }) }; - let task: gpui::Task> = cx.spawn_in(window, async move |this, cx| { + let task: Task> = cx.spawn_in(window, async move |this, cx| { let disk = disk.await; let had_clone = matches!(&disk, Ok(Some(_))); @@ -393,7 +388,6 @@ impl RepoDetailView { // Refresh the clone from the network in the background. // When it completes, update the refs and commit list. - // Loads started before a branch/tag switch are discarded via the generation. if !had_clone { return Ok(()); } @@ -408,15 +402,10 @@ impl RepoDetailView { }; // Best-effort, a fetch failure, e.g. offline, keeps the cached state. - // The state is already shown. signed_git::fetch_all(&repo).ok(); let worktree = repo.workdir().map(Path::to_path_buf); - // A fetch never moves a mirror's local branches. - // A push landing on the grasp servers would never show up. - // That covers own repo pushes from a checkout and updates fetched here. - // Fast-forward branches from the remote, like `git pull --ff-only`. - // Only the checked-out branch's worktree can change on disk. + let moved = match &worktree { Some(worktree) => { signed_git::fast_forward_branches(worktree).unwrap_or(false) @@ -441,10 +430,6 @@ impl RepoDetailView { .await; this.update_in(cx, |this, window, cx| { - if refresh_generation != this.ref_generation { - return; - } - if let Ok(Some((moved, branches, tags, current_branch, head_commit))) = refresh { let branches: Vec = branches.iter().map(Into::into).collect(); let tags: Vec = tags.iter().map(Into::into).collect(); @@ -619,7 +604,6 @@ impl RepoDetailView { } self.refs.switching_ref = true; - self.ref_generation += 1; cx.notify(); let checkout_name = name.clone(); diff --git a/docs/repo-view-refactor-plan.md b/docs/repo-view-refactor-plan.md deleted file mode 100644 index 1a5350e..0000000 --- a/docs/repo-view-refactor-plan.md +++ /dev/null @@ -1,291 +0,0 @@ -# Repo view refactor plan - -## Goal - -Make `crates/workspace/src/views/repo/` easy to navigate and change: - -- Each concern owns its own state (its own struct fields), instead of all concerns sharing one 38-field struct. -- Shared UI moves to the module that consumes it, so sibling views stop importing from `views::repo`. -- No behavior change. No new global state. No new store. The GPUI patterns already used in the repo (`Entity` + observe, `TreeState`, `ComboboxState`, `VirtualListScrollHandle`) stay the only patterns used. - -## Constraints - -- Follow `.rules`: no `unwrap` in production, no silently discarded errors, full-word names, comments explain "why" only. -- Do not over-engineer. Files and History become entities because they already render and run async independently. Refs and Banners stay plain field groups on the shell. -- Keep the shell as the single load/reconcile point. The clone/worktree and `ref_generation` belong to the shell, not to child views. -- `signed_ui` does **not** depend on `signed_git` (verified in `crates/signed_ui/Cargo.toml`). Anything that takes a `signed_git` type cannot move there. - -## Current state (verified) - -| File | Lines | Content | -|---|---|---| -| `mod.rs` | ~376 | `RepoDetailView` struct (38 fields), constructors, `render`, panel impls, `display_name` | -| `store.rs` | ~98 | `attach_store`, `apply_announcement`, `refresh_ready_statuses`, `refresh_statuses` | -| `actions.rs` | ~208 | action methods + `open_repo_panel` / `open_repo_item` free functions | -| `loading.rs` | ~435 | `load_repo`, `apply_repo_data`, `sync_ref_selector`, `clone_to_folder`, `load_repo_data` | -| `refs.rs` | ~279 | `switch_ref`, `restore_selection`, `reload_worktree`, `catch_up_worktree` | -| `files.rs` | ~480 | file tree, previews, markdown/code state, eviction | -| `history.rs` | ~228 | commits tab render + per-file and full commit walks | -| `header.rs` | ~715 | header render, maintainers, fork row, clone URL | -| `banners.rs` | ~315 | ready/push suggestion banners | -| `about.rs` | ~224 | about dialog | -| `init_dialog.rs` | ~202 | publish-to-NIP-34 dialog | -| `helpers.rs` | ~722 | `pub(crate)` grab bag: tree building, diff rendering, discussion UI, share targets, commit rows | - -### Problems - -1. **`RepoDetailView` is a god object.** 38 fields across six concerns. All 12 files are `impl RepoDetailView`, so any file can read/write any field. The file split added navigation cost without encapsulation. -2. **`helpers.rs` is an inverted dependency hub.** `views/issues/detail.rs` and `views/pull_requests/*.rs` import from `views::repo::helpers` for discussion UI, diff rows and commit rows. Sibling views reaching into `repo` is backwards. -3. **Two regions have independent async + render lifecycles** (file browser, commit history) but live as shell fields, sharing `worktree` and `ref_generation` by hand. - -### Existing good pattern - -`IssuesView` (`views/issues/mod.rs`): own struct (~13 fields), `cx.observe(&store, ..)`, `rebuild()` into local state, `Render`, no shell fields. The refactor brings `RepoDetailView` in line with this. - -## Target structure - -```mermaid -graph TD - Shell["RepoDetailView shell\nstore, dock_area, tabs, header,\nload orchestration, worktree, generation"] --> Files["Entity\nfiles.rs"] - Shell --> History["Entity\nhistory.rs"] - Shell --> Refs["RefSwitcher (plain)\nrefs.rs"] - Shell --> Banners["Banners (plain)\nbanners.rs"] - Files --> Store["Entity"] - History --> Store -``` - -Field ownership after the refactor: - -| Concern | Fields | Owner | -|---|---|---| -| Files | `tree_state, worktree_paths, md, code, readme_name, selected_file, files, file_order, preview_bytes, loading_files, commits, pending_commits, loading_commits` | `RepoFilesView` | -| History | `all_commits, loading_all_commits, item_sizes, scroll_handle` | `RepoHistoryView` | -| Refs | `branch_select, tag_select, ref_branches, ref_tags, switching_ref` | `RefSwitcher` | -| Banners | `banner_dismissed, ready_requested, ready_head, ready_statuses, push_statuses` | `Banners` | -| Shell | `focus_handle, dock_area, store, repo_started, active_tab, loading, error, head_commit, worktree, ref_generation, _subscriptions` | `RepoDetailView` (11 fields) | - -Shared modules after Phase 1: - -| New / changed module | Contents | Consumers | -|---|---|---| -| `views/tree.rs` | `TreeItemSeed`, `tree_items`, `build_tree_items`, `sorted_worktree_paths` + the 3 tree tests | repo files/loading, commit_diff | -| `views/commit_diff/mod.rs` | adds `DiffRow`, `diff_rows`, `render_diff_row`, `render_diff_line`, `find_item`, `GUTTER_WIDTH`, `DIFF_ROW_HEIGHT`, `commit_row`, `COMMIT_ROW_HEIGHT` | commit_diff, PR new, repo history | -| `views/discussion.rs` | `sidebar_title`, `sidebar_section`, `comments_section`, `comment_form`, `issue_roots`, `pr_roots` | issues detail, PR detail | -| `signed_ui/src/ref_selector.rs` | `ref_selector_trigger` | repo header, PR new | -| `repo/files.rs` | `code_language`, `is_markdown_path` (only used there) | repo files | -| `repo/header.rs` | `ShareTargets`, `truncate_naddr_link` (only used there) | repo header | - -`views/repo/helpers.rs` is deleted at the end of Phase 1. - ---- - -## Phase 0 - baseline - -No code. Record the current state so each later phase can be compared. - -1. `cargo fmt --all -- --check` -2. `cargo check --offline --workspace --all-targets` -3. `cargo test --offline -p workspace` -4. `cargo clippy --offline -p workspace --all-targets` - -Do not run plain `cargo` without `--offline`; the sandbox fails the git fetch and it looks like a dependency error. - ---- - -## Phase 1 - extract shared modules (dissolve `helpers.rs`) - -Low risk, no state moves. Land it as one commit. - -### 1.1 Create `crates/workspace/src/views/tree.rs` - -Move from `repo/helpers.rs`: `TreeItemSeed`, `tree_items`, `build_tree_items`, `sorted_worktree_paths`, and the three tests (`builds_nested_tree_from_flat_entries`, `tree_builder_handles_deep_nesting`, `tree_builder_merges_shared_prefixes`). - -- Add `pub(crate) mod tree;` to `views/mod.rs`. -- Update imports in `repo/loading.rs`, `repo/refs.rs`, `commit_diff/mod.rs` to `crate::views::tree::...`. - -### 1.2 Move diff and commit-row rendering into `views/commit_diff/mod.rs` - -Move from `repo/helpers.rs`: `GUTTER_WIDTH`, `DIFF_ROW_HEIGHT`, `DiffRow`, `diff_rows`, `render_diff_row`, `render_diff_line`, `find_item`, `COMMIT_ROW_HEIGHT`, `commit_row`. - -- `commit_diff/mod.rs` already owns `DiffPane` and depends on `signed_git`, so this is its natural home and keeps `signed_ui` free of a `signed_git` dependency. -- Update imports in `views/pull_requests/new.rs` and `repo/history.rs`. - -### 1.3 Create `crates/workspace/src/views/discussion.rs` - -Move from `repo/helpers.rs`: `sidebar_title`, `sidebar_section`, `comments_section`, `comment_form`, `issue_roots`, `pr_roots`. - -- Add `pub(crate) mod discussion;` to `views/mod.rs`. -- Update imports in `views/issues/detail.rs` and `views/pull_requests/detail.rs`. After this, neither imports from `views::repo`. - -### 1.4 Move `ref_selector_trigger` into `signed_ui` - -It takes `CustomIconName` (from `assets`) and `ComboboxTriggerContext` (from `gpui_component`); both are already `signed_ui` dependencies, so no dependency changes. - -- Add `crates/signed_ui/src/ref_selector.rs`, export it from `lib.rs`. -- Update imports in `repo/header.rs` and `views/pull_requests/new.rs`. - -### 1.5 Move `code_language` and `is_markdown_path` into `repo/files.rs` - -Only `repo/files.rs` uses them. Keep them private there. - -### 1.6 Move `ShareTargets` and `truncate_naddr_link` into `repo/header.rs` - -Only `repo/header.rs` uses them. Keep them private there. - -### 1.7 Delete `repo/helpers.rs` - -Remove `pub(super) mod helpers;` from `repo/mod.rs`. Confirm no `use ...repo::helpers` remains anywhere: - -``` -grep -rn "repo::helpers" crates/workspace/src -``` - -### Phase 1 validation - -`cargo fmt --all`, `cargo check --offline -p workspace --all-targets`, `cargo test --offline -p workspace`, `cargo clippy --offline -p workspace --all-targets`. - ---- - -## Phase 2 - extract `Entity` - -Largest win: removes 14 fields and most of the preview logic from the shell. - -### 2.1 Define the view - -In `repo/files.rs`, replace `impl RepoDetailView` with `pub(super) struct RepoFilesView` holding: `tree_state`, `worktree`, `worktree_paths`, `md`, `code`, `readme_name`, `selected_file`, `files`, `file_order`, `preview_bytes`, `loading_files`, `commits`, `pending_commits`, `loading_commits`. - -Move the supporting types and helpers from the current `files.rs` into the view: `FileContent`, `MarkdownView`, `CodeView`, `MAX_PREVIEW_BYTES`, `MAX_PREVIEWED_FILES`, `MAX_PREVIEW_CACHE_BYTES`, `source_hash`, `preview_spinner`, `render_tree_item`, `render_tree_column`, `render_content_column`, `set_markdown`, `markdown_element`, `set_code`, `code_element`, `open_file`, `drop_preview_of`, `evict_previews`. - -Move from `repo/history.rs`: `load_commit`, `load_commits` (the per-file commit map). - -### 2.2 Define the view's interface - -- `pub(super) fn new(window: &mut Window, cx: &mut Context) -> Self` - creates the `TreeState`. -- `pub(super) fn set_worktree(&mut self, path: PathBuf)`. -- `pub(super) fn apply_entries(&mut self, tree: Vec, paths: Vec, window, cx)` - used by `load_repo` / `reload_worktree` / `catch_up_worktree`. -- `pub(super) fn set_readme(&mut self, path: Option, bytes: Option>, cx)`. -- `pub(super) fn clear_previews(&mut self)` - branch switch. -- `pub(super) fn catch_up(&mut self, snapshot, window, cx) -> bool` - rebuild tree, drop removed previews, re-render README; returns whether anything changed. -- `impl Render for RepoFilesView`. -- `pub(super) fn pane_title(&self) -> SharedString` - `selected_file` or `readme_name` or `"Overview"`. - -### 2.3 Move the clone loading/error display out of the file view - -`render_content_column` currently shows "Cloning repository..." / a load error from `self.loading` and `self.error`, which are shell state. Move that decision to the shell's `render`: while `self.loading`, render a spinner in the tab body; when `self.error` is set, the existing `Alert` already covers it. `render_content_column` then handles only file previews and the README. - -### 2.4 Wire the shell - -- Add `files: Entity` to `RepoDetailView`. -- In `new_common`, `let files = cx.new(|cx| RepoFilesView::new(window, cx));`. -- In `render`, the Files tab body becomes `self.files.clone()`. -- In `load_repo` (`loading.rs`) and `reload_worktree` / `catch_up_worktree` (`refs.rs`), replace direct field writes with calls on `self.files`. -- Remove the now-unused `files.rs` imports from `mod.rs` and the moved fields from the struct and constructor. - -### Phase 2 validation - -Same commands. Manual: open explore repo, click files in the tree, open the README, switch branch (previews clear), switch back, confirm no spinner sticks. - ---- - -## Phase 3 - extract `Entity` - -### 3.1 Define the view - -In `repo/history.rs`, replace the commits-tab methods with `pub(super) struct RepoHistoryView` holding: `store: Entity`, `dock_area: WeakEntity`, `worktree: Option`, `all_commits`, `loading_all_commits`, `item_sizes`, `scroll_handle`. - -Move: `render_commits_tab` (becomes `impl Render`), `load_all_commits`, `open_commit_diff`. - -### 3.2 Display name - -`open_commit_diff` uses the shell's `display_name`. Extract the `display_name` logic from `RepoDetailView` into a free function in `repo/mod.rs`: - -```rust -pub(super) fn repo_display_name(store: &RepoStore) -> SharedString -``` - -It keeps the local-path fallback that `RepoStore::name()` does not have. Use it in the shell's `Panel::title`, in the header, and in `RepoHistoryView::open_commit_diff`. - -### 3.3 Interface - -- `pub(super) fn new(store, dock_area, window, cx) -> Self`. -- `pub(super) fn set_worktree(&mut self, path: Option)`. -- `pub(super) fn reload(&mut self, cx)` - clears `all_commits` and starts the walk (called when HEAD changes or the branch switches). -- `impl Render for RepoHistoryView`. - -### 3.4 Wire the shell - -- Add `history: Entity` to `RepoDetailView`; create it in `new_common`. -- In `render`, tab 1 becomes `self.history.clone()`. -- Replace `self.all_commits` / `self.loading_all_commits` / `self.item_sizes` writes in `load_repo`, `reload_worktree`, `catch_up_worktree`, and the header-commit pill path with `self.history.update(..)` calls. -- Remove the moved fields from the struct and constructor. - -### Phase 3 validation - -Same commands. Manual: open the Commits tab, scroll a long history, click a commit (diff panel opens), switch branch and confirm the list reloads. - ---- - -## Phase 4 - group `RefSwitcher` and `Banners` - -Plain structs on the shell. No entity, no observer changes. - -### 4.1 `RefSwitcher` - -Move into a `struct RefSwitcher { branch_select, tag_select, ref_branches, ref_tags, switching_ref }` field on the shell. Update `refs.rs` and `loading.rs` methods to read/write `self.refs.*`. `switch_ref` stays on the shell because it fans out to files, history and `head_commit`. - -`ref_generation` stays on the shell: it is shared with the files and history loads. - -### 4.2 `Banners` - -Move into a `struct Banners { dismissed, ready_requested, ready_head, ready_statuses, push_statuses }` field. `banners.rs` and `store.rs` methods keep their `impl RepoDetailView` shape but read/write `self.banners.*`. - -### Phase 4 validation - -Same commands. Manual: the ready-to-contribute banner appears and dismisses, the push banner appears for an owned repo, dismissing survives a store refresh. - ---- - -## Phase 5 - fold `store.rs` and tidy - -1. Move `attach_store`, `apply_announcement`, `refresh_ready_statuses`, `refresh_statuses` into `mod.rs` and delete `repo/store.rs`. -2. Remove `mod store;` from `repo/mod.rs`. -3. Confirm `mod.rs` reads as a shell: struct, constructors, load coordination, `render`, panel impls. -4. Final validation: - -``` -cargo fmt --all -cargo check --offline --workspace --all-targets -cargo test --offline --workspace -cargo clippy --offline --workspace --all-targets -``` - -## Validation (manual smoke, after each phase) - -- Open a repo from the explore list, then open an issue and a PR. -- Deep-link straight to an issue / PR without visiting the repo panel. -- Open a local repository (never announced). -- Initialize a local repo to NIP-34, confirm it leaves the sidebar's local section. -- Clone to folder; clone again before the first clone completes. -- Switch a branch and a tag; confirm previews and the commit list reset. -- Owned repo with unpushed commits: push banner, push, republish banner. - -## Boundary test for "done" - -- No file can touch fields it does not own. -- `repo/mod.rs` is a shell, roughly 200 lines. -- `grep -rn "views::repo::helpers" crates/workspace/src` returns nothing. -- `views/issues` and `views/pull_requests` have no `use ...views::repo`. - -## Non-goals - -- No behavior change; no UI redesign. -- No new global state, no new store, no changes to `signed_state` or `dock`. -- No more `impl RepoDetailView` chapters. New files own structs, not fragments of one struct. -- Do not move `commit_row` into `signed_ui`: it takes `signed_git::FileCommit` and `signed_ui` does not depend on `signed_git`. - -## Risks and open questions - -- **Async generation.** `ref_generation` discards stale loads. It stays on the shell; when the shell pushes a snapshot into a child view, the child must not start a new load that outlives the generation. Simplest rule: only the shell starts loads, child views only render and own per-file preview fetches keyed to the current worktree. -- **Files owns the per-file commit walk.** `load_commit`/`load_commits` move with the preview state, so the shell no longer coordinates them. Confirm the README commit lookup still works after the move. -- **History is small.** After moving `load_commit`/`load_commits` to Files, `history.rs` is ~150 lines. If an entity feels heavy for that, a plain `struct History` field is an acceptable fallback; the field ownership still improves. -- **`RepoStore::name()` vs `display_name`.** `RepoStore::name()` returns `Unknown` for local repos. The extracted `repo_display_name` must keep the local-path fallback so titles are unchanged.