diff --git a/apps/penpal/ERD.md b/apps/penpal/ERD.md index 60d5d300b..85f9cefce 100644 --- a/apps/penpal/ERD.md +++ b/apps/penpal/ERD.md @@ -91,6 +91,9 @@ see-also: - **E-PENPAL-WORKTREE-DISCOVERY**: Worktrees are discovered by parsing `git worktree list --porcelain` output. Each worktree gets a name, path, branch, and `IsMain` flag. The `refs/heads/` prefix is stripped from branch names. ← [P-PENPAL-WORKTREE](PRODUCT.md#P-PENPAL-WORKTREE) +- **E-PENPAL-WORKTREE-WATCH**: Worktree additions and removals are detected by the existing workspace directory watch — `git worktree add`/`remove` creates or deletes a sibling directory in the workspace, triggering the workspace rescan path. The rescan re-runs full project discovery (including `DiscoverWorktrees`), updates the cache, and broadcasts a `projects` SSE event so the frontend reflects the change. No additional inotify watches or subprocess calls are needed. `GitCommonDir` resolves the shared `.git` directory using pure filesystem reads (no `git rev-parse`). + ← [P-PENPAL-WORKTREE](PRODUCT.md#P-PENPAL-WORKTREE) + - **E-PENPAL-CLAUDE-PLANS-DETECT**: `DiscoverClaudePlans()` checks `~/.claude/plans/` for existence and at least one `.md` file. If found, a synthetic standalone project is injected. If the user already manually added the same path, a tree source is injected into the existing entry instead of duplicating. ← [P-PENPAL-CLAUDE-PLANS](PRODUCT.md#P-PENPAL-CLAUDE-PLANS) diff --git a/apps/penpal/PRODUCT.md b/apps/penpal/PRODUCT.md index 2ef61851f..46b27c6f1 100644 --- a/apps/penpal/PRODUCT.md +++ b/apps/penpal/PRODUCT.md @@ -22,7 +22,7 @@ Penpal is a desktop application and local web server for collaborative review of - **P-PENPAL-STANDALONE**: Users can add standalone projects (directories or individual files) outside of any workspace, via the home view "+" button or the `penpal open` CLI command. -- **P-PENPAL-WORKTREE**: Git worktrees for a project are discovered automatically. In the home view, multi-worktree projects expand to show each worktree as a child item with its branch name. In the project view, a worktree dropdown in the breadcrumb bar lets the user switch between worktrees. Each worktree has its own branch name and independent comment storage. +- **P-PENPAL-WORKTREE**: Git worktrees for a project are discovered automatically. In the home view, multi-worktree projects expand to show each worktree as a child item with its branch name. In the project view, a worktree dropdown in the breadcrumb bar lets the user switch between worktrees. Each worktree has its own branch name and independent comment storage. When worktrees are added or removed (via `git worktree add`/`remove`), the worktree list updates without restarting the server. - **P-PENPAL-DEDUP**: When multiple directories in a workspace share the same git repository (one is a worktree of the other), only the main worktree is shown as a project to avoid duplicates. diff --git a/apps/penpal/TESTING.md b/apps/penpal/TESTING.md index 4315fce92..77fa2b3ba 100644 --- a/apps/penpal/TESTING.md +++ b/apps/penpal/TESTING.md @@ -66,6 +66,7 @@ see-also: | Source Types — manual (P-PENPAL-SRC-MANUAL) | — | — | grouping_test.go (TestBuildFileGroups_ManualSourceDirHeadings) | — | | Cache & File Scanning (E-PENPAL-CACHE, SCAN) | cache_test.go (TestCheckAllProjectsHasFiles, TestProjectHasAnyMarkdown_IgnoresGitignore, TestProjectHasAnyMarkdown_SkipsVCSDirs, TestAllFiles_DeduplicatesAllMarkdown, TestEnsureProjectScanned_NoDuplicateScans, TestResolveFileInfo, TestUpsertFile, TestRemoveFile, TestRescanWith_PreservesUnchangedProjects, TestSourcesChanged) | — | — | — | | Worktree Support (P-PENPAL-WORKTREE) | discovery/worktree_test.go, cache/worktree_test.go | Layout.test.tsx | worktree_test.go (API + MCP) | — | +| Worktree Watch (E-PENPAL-WORKTREE-WATCH) | watcher_test.go | — | — | — | | Worktree Dropdown (P-PENPAL-PROJECT-WORKTREE-DROPDOWN) | — | Layout.test.tsx | — | — | | Git Integration (P-PENPAL-GIT-INFO) | — | — | — | — | | File List & Grouping (P-PENPAL-FILE-LIST) | — | ProjectPage.test.tsx | grouping_test.go, integration_test.go | — | diff --git a/apps/penpal/internal/discovery/worktree.go b/apps/penpal/internal/discovery/worktree.go index ce0af4e15..2212485bb 100644 --- a/apps/penpal/internal/discovery/worktree.go +++ b/apps/penpal/internal/discovery/worktree.go @@ -1,11 +1,54 @@ package discovery import ( + "os" "os/exec" "path/filepath" "strings" ) +// gitCommonDirFS resolves the shared .git directory using only filesystem +// reads — no subprocess. For a main worktree .git is a directory; for a +// linked worktree .git is a file containing "gitdir: " and the +// referenced gitdir contains a "commondir" file pointing back to the +// shared .git. +func gitCommonDirFS(projectPath string) string { + gitPath := filepath.Join(projectPath, ".git") + info, err := os.Lstat(gitPath) + if err != nil { + return "" + } + // Main worktree: .git is a directory — it IS the common dir. + if info.IsDir() { + return gitPath + } + // Linked worktree: .git is a file with "gitdir: ". + data, err := os.ReadFile(gitPath) + if err != nil { + return "" + } + line := strings.TrimSpace(string(data)) + if !strings.HasPrefix(line, "gitdir: ") { + return "" + } + gitDir := strings.TrimPrefix(line, "gitdir: ") + if !filepath.IsAbs(gitDir) { + gitDir = filepath.Join(projectPath, gitDir) + } + gitDir = filepath.Clean(gitDir) + // Read commondir file to find the shared .git directory. + cdPath := filepath.Join(gitDir, "commondir") + cdData, err := os.ReadFile(cdPath) + if err != nil { + return "" + } + commonDir := strings.TrimSpace(string(cdData)) + if !filepath.IsAbs(commonDir) { + commonDir = filepath.Join(gitDir, commonDir) + } + return filepath.Clean(commonDir) +} + // Worktree represents a git worktree associated with a project. type Worktree struct { Name string `json:"name"` // directory name (e.g., "fancy-name") @@ -81,64 +124,17 @@ func parseWorktreeList(projectPath string, output string) []Worktree { return worktrees } -// ResolveWorktree finds the worktree that contains the given absolute path. -// Returns the worktree name and the main project path, or empty strings if -// the path doesn't belong to any worktree. -func ResolveWorktree(projectPath string, absPath string) (worktreeName string, mainProjectPath string) { - absPath = filepath.Clean(absPath) - - // First check if this path is inside the main project - mainPath := filepath.Clean(projectPath) - if strings.HasPrefix(absPath, mainPath+"/") || absPath == mainPath { - // Check if it's inside a worktree subdirectory - worktrees := DiscoverWorktrees(projectPath) - for _, wt := range worktrees { - if !wt.IsMain && (strings.HasPrefix(absPath, wt.Path+"/") || absPath == wt.Path) { - return wt.Name, mainPath - } - } - return "", mainPath - } - - return "", "" -} - -// FindMainWorktree returns the path to the main worktree for a given path -// that might be inside a worktree. It reads the .git file to find the -// gitdir and traces back to the main worktree. -func FindMainWorktree(path string) string { - cmd := exec.Command("git", "-C", path, "rev-parse", "--git-common-dir") - out, err := cmd.Output() - if err != nil { +// gitWorktreesDir returns the path to the .git/worktrees/ directory for the +// repository that projectPath belongs to, or "" if it doesn't exist. +// Uses pure filesystem reads via gitCommonDirFS — no subprocess calls. +func gitWorktreesDir(projectPath string) string { + commonDir := gitCommonDirFS(projectPath) + if commonDir == "" { return "" } - commonDir := strings.TrimSpace(string(out)) - if commonDir == "" || commonDir == "." { - return "" + wtDir := filepath.Join(commonDir, "worktrees") + if info, err := os.Stat(wtDir); err == nil && info.IsDir() { + return wtDir } - - // commonDir is the .git directory of the main worktree - // If it's relative, resolve it relative to the path - if !filepath.IsAbs(commonDir) { - // Get the actual git dir for this worktree first - cmd2 := exec.Command("git", "-C", path, "rev-parse", "--git-dir") - out2, err := cmd2.Output() - if err != nil { - return "" - } - gitDir := strings.TrimSpace(string(out2)) - if !filepath.IsAbs(gitDir) { - gitDir = filepath.Join(path, gitDir) - } - commonDir = filepath.Join(gitDir, commonDir) - } - - commonDir = filepath.Clean(commonDir) - - // The main worktree is the parent of the .git directory - if filepath.Base(commonDir) == ".git" { - return filepath.Dir(commonDir) - } - return "" } diff --git a/apps/penpal/internal/discovery/worktree_test.go b/apps/penpal/internal/discovery/worktree_test.go index 384859191..716f6834a 100644 --- a/apps/penpal/internal/discovery/worktree_test.go +++ b/apps/penpal/internal/discovery/worktree_test.go @@ -1,6 +1,9 @@ package discovery import ( + "os" + "os/exec" + "path/filepath" "testing" ) @@ -100,3 +103,144 @@ func TestParseWorktreeList_BranchStripping(t *testing.T) { t.Errorf("wt branch = %q, want %q", got[1].Branch, "feature/nested") } } + +// initGitRepo creates a git repo in dir with an initial commit. +func initGitRepo(t *testing.T, dir string) { + t.Helper() + for _, args := range [][]string{ + {"init"}, + {"config", "user.email", "test@test.com"}, + {"config", "user.name", "Test"}, + {"commit", "--allow-empty", "-m", "init"}, + } { + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } +} + +// resolveSymlinks resolves symlinks in a path for reliable comparison on macOS +// where /var → /private/var. +func resolveSymlinks(t *testing.T, path string) string { + t.Helper() + resolved, err := filepath.EvalSymlinks(path) + if err != nil { + t.Fatalf("EvalSymlinks(%q): %v", path, err) + } + return resolved +} + +// E-PENPAL-WORKTREE-WATCH: verifies gitWorktreesDir returns the .git/worktrees/ dir for a repo with worktrees. +func TestWorktreesDir_MainWorktree(t *testing.T) { + mainDir := resolveSymlinks(t, t.TempDir()) + initGitRepo(t, mainDir) + + // Before adding a worktree, the dir doesn't exist + if got := gitWorktreesDir(mainDir); got != "" { + t.Fatalf("expected empty before worktree add, got %q", got) + } + + // Add a worktree + wtDir := filepath.Join(resolveSymlinks(t, t.TempDir()), "my-worktree") + cmd := exec.Command("git", "-C", mainDir, "worktree", "add", "-b", "test-branch", wtDir) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git worktree add: %v\n%s", err, out) + } + + // Now gitWorktreesDir should return the .git/worktrees/ path + got := gitWorktreesDir(mainDir) + want := filepath.Join(mainDir, ".git", "worktrees") + if got != want { + t.Errorf("gitWorktreesDir(main) = %q, want %q", got, want) + } + + // It should also work when called from the linked worktree + got2 := gitWorktreesDir(wtDir) + if got2 != want { + t.Errorf("gitWorktreesDir(linked) = %q, want %q", got2, want) + } +} + +// E-PENPAL-WORKTREE-WATCH: verifies gitWorktreesDir returns "" for a non-git directory. +func TestWorktreesDir_NotGitRepo(t *testing.T) { + dir := t.TempDir() + if got := gitWorktreesDir(dir); got != "" { + t.Errorf("gitWorktreesDir(non-git) = %q, want empty", got) + } +} + +// E-PENPAL-WORKTREE-WATCH: verifies gitWorktreesDir returns "" for a repo with no worktrees. +func TestWorktreesDir_NoWorktrees(t *testing.T) { + dir := t.TempDir() + initGitRepo(t, dir) + if got := gitWorktreesDir(dir); got != "" { + t.Errorf("gitWorktreesDir(no worktrees) = %q, want empty", got) + } +} + +// E-PENPAL-WORKTREE-WATCH: verifies worktree directory appears after git worktree add +// and disappears after git worktree remove. +func TestWorktreesDir_AddRemoveCycle(t *testing.T) { + mainDir := resolveSymlinks(t, t.TempDir()) + initGitRepo(t, mainDir) + + wtPath := filepath.Join(resolveSymlinks(t, t.TempDir()), "wt") + cmd := exec.Command("git", "-C", mainDir, "worktree", "add", "-b", "wt-branch", wtPath) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git worktree add: %v\n%s", err, out) + } + + wtDir := gitWorktreesDir(mainDir) + if wtDir == "" { + t.Fatal("expected non-empty after add") + } + + // Verify the specific worktree entry exists + entries, err := os.ReadDir(wtDir) + if err != nil { + t.Fatal(err) + } + found := false + for _, e := range entries { + if e.Name() == filepath.Base(wtPath) { + found = true + } + } + if !found { + t.Errorf("expected entry %q in %s", filepath.Base(wtPath), wtDir) + } + + // Remove the worktree + cmd = exec.Command("git", "-C", mainDir, "worktree", "remove", wtPath) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git worktree remove: %v\n%s", err, out) + } + + // After removing the last worktree, the worktrees/ dir should be gone + if got := gitWorktreesDir(mainDir); got != "" { + t.Errorf("expected empty after removing last worktree, got %q", got) + } +} + +// E-PENPAL-WORKTREE-WATCH: verifies gitCommonDirFS returns "" for malformed .git file. +func TestGitCommonDirFS_MalformedGitFile(t *testing.T) { + dir := t.TempDir() + // .git file with no "gitdir:" prefix + os.WriteFile(filepath.Join(dir, ".git"), []byte("not a gitdir line\n"), 0o644) + if got := gitCommonDirFS(dir); got != "" { + t.Errorf("expected empty for malformed .git file, got %q", got) + } +} + +// E-PENPAL-WORKTREE-WATCH: verifies gitCommonDirFS returns "" when commondir file is missing. +func TestGitCommonDirFS_MissingCommondir(t *testing.T) { + dir := t.TempDir() + gitDir := filepath.Join(dir, "fake-gitdir") + os.MkdirAll(gitDir, 0o755) + // .git file points to a valid directory but commondir file doesn't exist + os.WriteFile(filepath.Join(dir, ".git"), []byte("gitdir: "+gitDir+"\n"), 0o644) + if got := gitCommonDirFS(dir); got != "" { + t.Errorf("expected empty for missing commondir, got %q", got) + } +} diff --git a/apps/penpal/internal/watcher/watcher.go b/apps/penpal/internal/watcher/watcher.go index 1a85700f0..ab4796c37 100644 --- a/apps/penpal/internal/watcher/watcher.go +++ b/apps/penpal/internal/watcher/watcher.go @@ -71,6 +71,9 @@ type Watcher struct { baseWatched map[string]struct{} dynamicWatched map[string]struct{} + // Serializes rediscoverProjects calls from concurrent debounce timers. + rediscoverMu sync.Mutex + // E-PENPAL-WATCHER: accumulating debounce for incremental file updates. // Collects per-file paths and ops during the debounce window, then applies // incremental cache mutations instead of full project walks. @@ -364,6 +367,7 @@ func (w *Watcher) syncBaseWatchesLocked(workspacePaths []string, projects []disc } w.baseWatched[path] = struct{}{} } + } func (w *Watcher) syncDynamicWatchesLocked() { @@ -484,27 +488,42 @@ func (w *Watcher) loop() { } } +// rediscoverProjects runs full project discovery, updates the cache and watches, +// and broadcasts a projects-changed SSE event. Serialized by rediscoverMu so +// concurrent debounce timers don't interleave cache updates. +func (w *Watcher) rediscoverProjects() { + if w.discoverFn == nil { + return + } + w.rediscoverMu.Lock() + defer w.rediscoverMu.Unlock() + + projects, err := w.discoverFn() + if err != nil { + log.Printf("Warning: project discovery failed: %v", err) + return + } + w.cache.RescanWith(projects) + w.focusMu.Lock() + w.syncBaseWatchesLocked(w.workspacePaths, projects) + w.syncDynamicWatchesLocked() + w.focusMu.Unlock() + w.Broadcast(Event{Type: EventProjectsChanged}) +} + // E-PENPAL-WATCHER: routes fsnotify events to project/workspace refresh or SSE broadcast. func (w *Watcher) handleEvent(event fsnotify.Event) { path := filepath.Clean(event.Name) + parentDir := filepath.Clean(filepath.Dir(path)) + + // E-PENPAL-WORKTREE-WATCH: worktree add/remove is detected by the workspace + // directory watch — the new worktree directory appears as a sibling project, + // triggering the workspace rescan path below. // Check if this is a change in a workspace directory (new/removed project) - parentDir := filepath.Clean(filepath.Dir(path)) for _, ws := range w.workspacePaths { if parentDir == filepath.Clean(ws) { - w.debounceRefresh("workspace:"+ws, func() { - if w.discoverFn != nil { - projects, err := w.discoverFn() - if err == nil { - w.cache.RescanWith(projects) - w.focusMu.Lock() - w.syncBaseWatchesLocked(w.workspacePaths, projects) - w.syncDynamicWatchesLocked() - w.focusMu.Unlock() - w.Broadcast(Event{Type: EventProjectsChanged}) - } - } - }) + w.debounceRefresh("workspace:"+ws, w.rediscoverProjects) return } } @@ -535,19 +554,7 @@ func (w *Watcher) handleEvent(event fsnotify.Event) { if matched { for _, p := range w.cache.Projects() { if parentDir == filepath.Clean(p.Path) { - w.debounceRefresh("sources:"+p.QualifiedName(), func() { - if w.discoverFn != nil { - projects, err := w.discoverFn() - if err == nil { - w.cache.RescanWith(projects) - w.focusMu.Lock() - w.syncBaseWatchesLocked(w.workspacePaths, projects) - w.syncDynamicWatchesLocked() - w.focusMu.Unlock() - w.Broadcast(Event{Type: EventProjectsChanged}) - } - } - }) + w.debounceRefresh("sources:"+p.QualifiedName(), w.rediscoverProjects) return } } diff --git a/apps/penpal/internal/watcher/watcher_test.go b/apps/penpal/internal/watcher/watcher_test.go index 4e30fc75c..8a0229720 100644 --- a/apps/penpal/internal/watcher/watcher_test.go +++ b/apps/penpal/internal/watcher/watcher_test.go @@ -1,11 +1,14 @@ package watcher import ( + "fmt" "os" "path/filepath" "sort" "testing" + "time" + "github.com/fsnotify/fsnotify" "github.com/loganj/penpal/internal/cache" "github.com/loganj/penpal/internal/discovery" ) @@ -282,6 +285,164 @@ func TestWindowFocusUnionAcrossWindows(t *testing.T) { assertWatched(t, w, thoughtsDir2, true, "window B still keeps proj2 watched") } +// E-PENPAL-WORKTREE-WATCH: verifies that rediscoverProjects updates the cache with +// new project data from discoverFn and broadcasts a projects-changed event. +func TestRediscoverProjects_UpdatesCache(t *testing.T) { + c := cache.New() + original := discovery.Project{Name: "repo", Path: "/tmp/repo"} + c.SetProjects([]discovery.Project{original}) + + w, err := New(c, nil) + if err != nil { + t.Fatal(err) + } + defer w.Stop() + + updated := discovery.Project{ + Name: "repo", Path: "/tmp/repo", + Worktrees: []discovery.Worktree{ + {Name: "repo", Path: "/tmp/repo", Branch: "main", IsMain: true}, + {Name: "feature", Path: "/tmp/wt/feature", Branch: "feature", IsMain: false}, + }, + } + w.discoverFn = func() ([]discovery.Project, error) { + return []discovery.Project{updated}, nil + } + + events := w.Subscribe() + defer w.Unsubscribe(events) + + w.rediscoverProjects() + + // Verify cache was updated with new worktree data + projects := c.Projects() + if len(projects) != 1 { + t.Fatalf("expected 1 project, got %d", len(projects)) + } + if len(projects[0].Worktrees) != 2 { + t.Errorf("expected 2 worktrees in cache, got %d", len(projects[0].Worktrees)) + } + + // Verify a projects-changed event was broadcast + select { + case evt := <-events: + if evt.Type != EventProjectsChanged { + t.Errorf("expected EventProjectsChanged, got %v", evt.Type) + } + case <-time.After(500 * time.Millisecond): + t.Fatal("expected projects-changed event to be broadcast") + } +} + +// E-PENPAL-WATCHER: verifies that non-.md file events are filtered out +// and do not trigger project rescans. +func TestEventFilter_NonMdFilesIgnored(t *testing.T) { + projDir := t.TempDir() + thoughtsDir := filepath.Join(projDir, "thoughts") + os.MkdirAll(thoughtsDir, 0o755) + + project := discovery.Project{ + Name: "proj", Path: projDir, + Sources: []discovery.FileSource{{ + Name: "thoughts", Type: "tree", SourceTypeName: "thoughts", + RootPath: thoughtsDir, Auto: true, + }}, + } + + c := cache.New() + c.SetProjects([]discovery.Project{project}) + + w, err := New(c, nil) + if err != nil { + t.Fatal(err) + } + defer w.Stop() + + events := w.Subscribe() + defer w.Unsubscribe(events) + + // Focus the project so the thoughts dir is watched and events reach the filter + w.FocusProject("proj") + + // Create event for a .js file should NOT trigger a files-changed broadcast + w.handleEvent(fsnotify.Event{ + Name: filepath.Join(thoughtsDir, "script.js"), + Op: fsnotify.Create, + }) + + select { + case evt := <-events: + t.Fatalf("expected no event for .js Create, got %v", evt) + case <-time.After(200 * time.Millisecond): + // good — no event + } + + // Write event for a .js file should also be filtered out + w.handleEvent(fsnotify.Event{ + Name: filepath.Join(thoughtsDir, "script.js"), + Op: fsnotify.Write, + }) + + select { + case evt := <-events: + t.Fatalf("expected no event for .js Write, got %v", evt) + case <-time.After(200 * time.Millisecond): + // good — no event + } + + // Create event for a .md file SHOULD trigger a files-changed broadcast + w.handleEvent(fsnotify.Event{ + Name: filepath.Join(thoughtsDir, "notes.md"), + Op: fsnotify.Create, + }) + + select { + case evt := <-events: + if evt.Type != EventFilesChanged { + t.Errorf("expected EventFilesChanged, got %v", evt.Type) + } + case <-time.After(500 * time.Millisecond): + t.Fatal("expected files-changed event for .md file") + } +} + +// E-PENPAL-WORKTREE-WATCH: verifies that rediscoverProjects does not update the +// cache or broadcast events when discoverFn returns an error. +func TestRediscoverProjects_ErrorNoUpdate(t *testing.T) { + c := cache.New() + original := discovery.Project{Name: "repo", Path: "/tmp/repo"} + c.SetProjects([]discovery.Project{original}) + + w, err := New(c, nil) + if err != nil { + t.Fatal(err) + } + defer w.Stop() + + w.discoverFn = func() ([]discovery.Project, error) { + return nil, fmt.Errorf("network error") + } + + events := w.Subscribe() + defer w.Unsubscribe(events) + + w.rediscoverProjects() + + // Cache should still have the original project, not be wiped + projects := c.Projects() + if len(projects) != 1 || projects[0].Name != "repo" { + t.Errorf("expected original project preserved, got %+v", projects) + } + + // No event should be broadcast + select { + case evt := <-events: + t.Fatalf("expected no event on error, got %v", evt) + case <-time.After(200 * time.Millisecond): + // good — no event + } +} + func assertWatched(t *testing.T, w *Watcher, dir string, expected bool, context string) { t.Helper() watched := w.watcher.WatchList()