From 2e67f286e9e2fcf9b7f041f1dc49e6229b0a788e Mon Sep 17 00:00:00 2001 From: Bruno Bornsztein Date: Tue, 23 Jun 2026 08:59:44 -0500 Subject: [PATCH 1/3] feat(detail): add read-only git file & diff viewer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Press `v` in the task detail view to open a keyboard-driven, read-only viewer of what the task's agent changed — without leaving the TUI: - A changed-file tree (left) scoped to the task's worktree, listing files the branch changed vs its base (merge-base of the source/default branch), including committed, staged, unstaged, and untracked files. - A content pane (the existing viewport) showing the selected file's unified diff with chroma syntax highlighting; `tab` toggles to a rendered view of the working-tree file, with glamour-rendered markdown for .md files. - up/down navigate files, j/k/wheel scroll the diff, esc closes. The viewer renders entirely inside the existing detail box, so the tmux claude/shell panes and their layout are untouched. All content writes go through setViewportContent() and every bit of viewer display state is folded into viewSignature(), so the View() render cache stays correct. File-list and per-file content loading happen on goroutines (matching the existing async pane-setup pattern) to keep the UI thread responsive on large diffs. Co-Authored-By: Claude Opus 4.8 --- go.mod | 2 +- internal/ui/app.go | 18 + internal/ui/detail.go | 70 +++- internal/ui/diffviewer.go | 695 +++++++++++++++++++++++++++++++++ internal/ui/diffviewer_test.go | 334 ++++++++++++++++ 5 files changed, 1116 insertions(+), 3 deletions(-) create mode 100644 internal/ui/diffviewer.go create mode 100644 internal/ui/diffviewer_test.go diff --git a/go.mod b/go.mod index c5c91a08..6a4e8679 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module github.com/bborn/workflow go 1.25.0 require ( + github.com/alecthomas/chroma/v2 v2.20.0 github.com/charmbracelet/bubbles v1.0.0 github.com/charmbracelet/bubbletea v1.3.10 github.com/charmbracelet/glamour v1.0.0 @@ -20,7 +21,6 @@ require ( ) require ( - github.com/alecthomas/chroma/v2 v2.20.0 // indirect github.com/anmitsu/go-shlex v0.0.0-20200514113438-38f4b401e2be // indirect github.com/atotto/clipboard v0.1.4 // indirect github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect diff --git a/internal/ui/app.go b/internal/ui/app.go index 4c4a73a9..6f185125 100644 --- a/internal/ui/app.go +++ b/internal/ui/app.go @@ -101,6 +101,8 @@ type KeyMap struct { // Spotlight mode Spotlight key.Binding SpotlightSync key.Binding + // Read-only file/diff viewer in the detail view + ViewDiff key.Binding } // ShortHelp returns key bindings to show in the mini help. @@ -281,6 +283,10 @@ func DefaultKeyMap() KeyMap { key.WithKeys("F"), key.WithHelp("F", "spotlight sync"), ), + ViewDiff: key.NewBinding( + key.WithKeys("v"), + key.WithHelp("v", "review changes"), + ), } } @@ -2479,6 +2485,15 @@ func (m *AppModel) updateDetail(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil } + // When the read-only file/diff viewer is open, let it consume navigation + // keys (file up/down, mode toggle, esc to close) before the normal detail + // keybindings. Keys it doesn't consume (j/k scrolling, etc.) fall through. + if m.detailView != nil && m.detailView.FileViewerActive() { + if handled, vcmd := m.detailView.HandleFileViewerKey(keyMsg); handled { + return m, vcmd + } + } + if key.Matches(keyMsg, m.keys.Back) { m.currentView = ViewDashboard // Clear origin column when exiting detail view @@ -2610,6 +2625,9 @@ func (m *AppModel) updateDetail(msg tea.Msg) (tea.Model, tea.Cmd) { m.detailView.ToggleShellPane() return m, nil } + if key.Matches(keyMsg, m.keys.ViewDiff) && m.detailView != nil && m.selectedTask != nil { + return m, m.detailView.OpenFileViewer() + } // Arrow key navigation to prev/next task in the same column // j/k keys are passed through to the viewport for scrolling diff --git a/internal/ui/detail.go b/internal/ui/detail.go index 00950c70..5cebd0ee 100644 --- a/internal/ui/detail.go +++ b/internal/ui/detail.go @@ -207,6 +207,12 @@ type DetailModel struct { relatedTasksLoading bool // true while loading related tasks relatedTasksLoaded bool // true once loaded (even if empty) lastRelatedSearch string // cache key for related task search + + // Read-only git file & diff viewer (see diffviewer.go). When active, the + // detail box renders a changed-file tree plus the selected file's diff + // instead of the task body. All of its display state is folded into + // viewSignature so the View render cache stays correct. + diff *diffViewer } // Message types for async pane loading @@ -768,7 +774,7 @@ func (m *DetailModel) initViewport() { // Just use full height - the TUI pane will be resized by tmux } - m.viewport = viewport.New(m.width-4, vpHeight) + m.viewport = viewport.New(m.contentViewportWidth(), vpHeight) m.setViewportContent() m.ready = true } @@ -780,7 +786,7 @@ func (m *DetailModel) SetSize(width, height int) { if m.ready { headerHeight := 6 footerHeight := 2 - m.viewport.Width = width - 4 + m.viewport.Width = m.contentViewportWidth() m.viewport.Height = height - headerHeight - footerHeight m.setViewportContent() } @@ -855,6 +861,15 @@ func (m *DetailModel) Update(msg tea.Msg) (*DetailModel, tea.Cmd) { log.Info("panesRefreshMsg: refreshing panes for task %d", m.task.ID) // Re-start the async pane setup return m, m.startPanesAsync() + + case diffFilesLoadedMsg: + // Changed-file list for the file/diff viewer loaded. + return m, m.HandleDiffFilesLoaded(msg) + + case diffContentLoadedMsg: + // Selected file's diff/rendered content loaded. + m.HandleDiffContentLoaded(msg) + return m, nil } // Pass all messages to viewport for scrolling support @@ -2362,6 +2377,14 @@ func (m *DetailModel) View() string { content := m.viewport.View() + // When the file/diff viewer is open, render the changed-file tree to the + // left of the (narrowed) content viewport. + if m.diff != nil && m.diff.active { + tree := m.renderDiffTree(m.viewport.Height) + gutter := lipgloss.NewStyle().Width(1).Height(m.viewport.Height).Render("") + content = lipgloss.JoinHorizontal(lipgloss.Top, tree, gutter, content) + } + // Use dimmed border when unfocused borderColor := ColorPrimary if !m.focused { @@ -2457,6 +2480,16 @@ func (m *DetailModel) viewSignature(header, help string) uint64 { h.int(m.viewport.Height) h.int(m.viewport.TotalLineCount()) h.int(m.viewport.VisibleLineCount()) + // File/diff viewer state drives the left tree column rendered in View(); + // fold it in so selecting a file or toggling the viewer busts the cache. + if m.diff != nil { + h.boolean(m.diff.active) + h.boolean(m.diff.loading) + h.int(m.diff.selected) + h.int(len(m.diff.files)) + h.boolean(m.diff.showRendered) + h.str(m.diff.loadErr) + } h.str(header) h.str(help) return h.h @@ -2801,6 +2834,11 @@ func (m *DetailModel) computeLogHash() uint64 { func (m *DetailModel) renderContent() string { t := m.task + // File/diff viewer takes over the content pane when open. + if m.diff != nil && m.diff.active { + return m.renderDiffContent() + } + // Check if we can use cached content // Note: We don't cache when related tasks are loading/changing logHash := m.computeLogHash() @@ -3031,6 +3069,29 @@ func (m *DetailModel) renderHelp() string { disabled bool // When disabled, always show grayed out } + // File/diff viewer has its own, focused help line. + if m.diff != nil && m.diff.active { + viewerKeys := []helpKey{ + {IconArrowUp() + "/" + IconArrowDown(), "file", len(m.diff.files) == 0}, + {"j/k/wheel", "scroll", false}, + {"tab", "diff/rendered", len(m.diff.files) == 0}, + {"esc", "close", false}, + } + var vh string + for i, k := range viewerKeys { + if i > 0 { + vh += " " + } + if k.disabled || !m.focused { + vh += lipgloss.NewStyle().Foreground(lipgloss.Color("#6B7280")).Render(k.key) + " " + + lipgloss.NewStyle().Foreground(lipgloss.Color("#4B5563")).Render(k.desc) + } else { + vh += HelpKey.Render(k.key) + " " + HelpDesc.Render(k.desc) + } + } + return HelpBar.Render(vh) + } + // Check if navigation is available (more than 1 task in column) hasNavigation := m.totalInColumn > 1 @@ -3100,6 +3161,11 @@ func (m *DetailModel) renderHelp() string { } } + // Review changes (file/diff viewer) when the task has a worktree + if m.task != nil && m.task.WorktreePath != "" { + keys = append(keys, helpKey{"v", "review changes", false}) + } + // Open PR shortcut (only when task has a PR) if m.task != nil && m.task.PRURL != "" { keys = append(keys, helpKey{"G", "open PR", false}) diff --git a/internal/ui/diffviewer.go b/internal/ui/diffviewer.go new file mode 100644 index 00000000..bb74e56d --- /dev/null +++ b/internal/ui/diffviewer.go @@ -0,0 +1,695 @@ +package ui + +// Git-aware, read-only file & diff viewer for the task detail view. +// +// This renders entirely inside the existing DetailModel viewport box: a file +// tree (left column) listing the files the task branch changed vs its base, and +// a content pane (the scrollable viewport) showing the unified diff for the +// selected file — with chroma syntax highlighting, and glamour-rendered markdown +// for .md files when toggled to "rendered" mode. +// +// The viewer never writes to the viewport directly: every content update goes +// through DetailModel.setViewportContent(), and all of its display state is +// folded into DetailModel.viewSignature(), so the View() render cache stays +// correct (see detail.go). + +import ( + "fmt" + "os" + osExec "os/exec" + "path/filepath" + "strings" + + tea "github.com/charmbracelet/bubbletea" + "github.com/charmbracelet/glamour" + "github.com/charmbracelet/lipgloss" + + "github.com/alecthomas/chroma/v2" + "github.com/alecthomas/chroma/v2/formatters" + "github.com/alecthomas/chroma/v2/lexers" + "github.com/alecthomas/chroma/v2/styles" +) + +// maxDiffBytes bounds how much text we render for a single file so a huge diff +// or generated file can't lock up the UI thread during highlighting. +const maxDiffBytes = 400 * 1024 + +// diffFileEntry is one changed file in the task branch. +type diffFileEntry struct { + path string // path relative to the worktree root + status string // git status letter: M, A, D, R, C, or "?" for untracked +} + +// diffViewer holds all state for the detail view's file/diff viewer. A nil or +// inactive diffViewer means the detail view renders its normal task content. +type diffViewer struct { + active bool + + worktree string + base string // resolved base ref (a merge-base sha, or "HEAD" fallback) + baseLabel string // human label for the base, e.g. "main" + + loading bool // file list is loading + loadErr string // file list load error (user-visible) + + files []diffFileEntry + selected int + + showRendered bool // false = unified diff, true = rendered file content + + // Content pane state for the currently selected file. + contentLoading bool + contentPath string // path the rendered content belongs to + contentMode bool // showRendered value the content was rendered for + rendered string // final, ready-to-display content string +} + +// --- messages ------------------------------------------------------------- + +type diffFilesLoadedMsg struct { + taskID int64 + base string + baseLabel string + files []diffFileEntry + err error +} + +type diffContentLoadedMsg struct { + taskID int64 + path string + showRendered bool + // raw text plus a hint about how to render it on the main thread + text string + kind diffContentKind + isMD bool + err error + empty bool // no changes / nothing to show +} + +type diffContentKind int + +const ( + diffKindDiff diffContentKind = iota // unified diff text -> chroma "diff" + diffKindFile // raw file content -> chroma by name / glamour +) + +// --- public hooks used by DetailModel / app.go ---------------------------- + +// FileViewerActive reports whether the file/diff viewer is currently open. +func (m *DetailModel) FileViewerActive() bool { + return m.diff != nil && m.diff.active +} + +// OpenFileViewer opens the read-only file/diff viewer for the task's worktree +// and returns the command that asynchronously loads the changed-file list. +func (m *DetailModel) OpenFileViewer() tea.Cmd { + if m.task == nil { + return nil + } + if m.diff == nil { + m.diff = &diffViewer{} + } + d := m.diff + d.active = true + d.worktree = m.task.WorktreePath + d.loading = true + d.loadErr = "" + d.files = nil + d.selected = 0 + d.showRendered = false + d.rendered = "" + d.contentPath = "" + d.contentLoading = false + + // Narrow the viewport to the content column and reset scroll. + if m.ready { + m.viewport.Width = m.contentViewportWidth() + m.viewport.GotoTop() + m.setViewportContent() + } + + if d.worktree == "" { + // Nothing to load; renderDiffContent shows an empty state. + d.loading = false + return nil + } + taskID := m.task.ID + worktree := d.worktree + source := m.task.SourceBranch + return func() tea.Msg { + base, label, files, err := loadChangedFiles(worktree, source) + return diffFilesLoadedMsg{taskID: taskID, base: base, baseLabel: label, files: files, err: err} + } +} + +// CloseFileViewer closes the viewer and restores normal task content. +func (m *DetailModel) CloseFileViewer() { + if m.diff == nil || !m.diff.active { + return + } + m.diff.active = false + if m.ready { + m.viewport.Width = m.contentViewportWidth() + m.viewport.GotoTop() + m.setViewportContent() + } +} + +// HandleFileViewerKey handles a key while the viewer is open. It returns whether +// the key was consumed and any command to run. Keys it does not consume (e.g. +// j/k scrolling) fall through to the normal viewport handling in app.go. +func (m *DetailModel) HandleFileViewerKey(msg tea.KeyMsg) (bool, tea.Cmd) { + if m.diff == nil || !m.diff.active { + return false, nil + } + d := m.diff + switch msg.String() { + case "esc", "v", "q": + m.CloseFileViewer() + return true, nil + case "up": + if len(d.files) > 0 { + d.selected-- + if d.selected < 0 { + d.selected = len(d.files) - 1 + } + return true, m.loadSelectedFileContent() + } + return true, nil + case "down": + if len(d.files) > 0 { + d.selected++ + if d.selected >= len(d.files) { + d.selected = 0 + } + return true, m.loadSelectedFileContent() + } + return true, nil + case "tab", "enter", " ": + if len(d.files) > 0 { + d.showRendered = !d.showRendered + return true, m.loadSelectedFileContent() + } + return true, nil + } + return false, nil +} + +// HandleDiffFilesLoaded applies an async file-list load result. +func (m *DetailModel) HandleDiffFilesLoaded(msg diffFilesLoadedMsg) tea.Cmd { + if m.diff == nil || !m.diff.active || m.task == nil || msg.taskID != m.task.ID { + return nil + } + d := m.diff + d.loading = false + if msg.err != nil { + d.loadErr = msg.err.Error() + m.setViewportContent() + return nil + } + d.base = msg.base + d.baseLabel = msg.baseLabel + d.files = msg.files + d.selected = 0 + m.setViewportContent() + if len(d.files) > 0 { + return m.loadSelectedFileContent() + } + return nil +} + +// HandleDiffContentLoaded applies an async per-file content load result and +// renders it (chroma / glamour) on the main thread. +func (m *DetailModel) HandleDiffContentLoaded(msg diffContentLoadedMsg) { + if m.diff == nil || !m.diff.active || m.task == nil || msg.taskID != m.task.ID { + return + } + d := m.diff + // Ignore stale results (user already moved on / toggled mode). + if msg.path != d.selectedPath() || msg.showRendered != d.showRendered { + return + } + d.contentLoading = false + d.contentPath = msg.path + d.contentMode = msg.showRendered + switch { + case msg.err != nil: + d.rendered = lipgloss.NewStyle().Foreground(lipgloss.Color("#E06C75")). + Render("Failed to load: " + msg.err.Error()) + case msg.empty: + d.rendered = Dim.Render("(no textual changes)") + case msg.kind == diffKindFile && msg.isMD: + d.rendered = m.renderViewerMarkdown(msg.text) + case msg.kind == diffKindFile: + d.rendered = highlightSource(msg.text, msg.path) + default: // diff + d.rendered = highlightSource(msg.text, "diff.diff") + } + m.viewport.GotoTop() + m.setViewportContent() +} + +// loadSelectedFileContent kicks off async loading of the selected file's content +// (diff or rendered file) and shows a loading placeholder immediately. +func (m *DetailModel) loadSelectedFileContent() tea.Cmd { + d := m.diff + if d == nil || len(d.files) == 0 { + return nil + } + if d.selected < 0 || d.selected >= len(d.files) { + d.selected = 0 + } + entry := d.files[d.selected] + d.contentLoading = true + d.rendered = Dim.Render("Loading " + entry.path + "…") + m.viewport.GotoTop() + m.setViewportContent() + + taskID := m.task.ID + worktree := d.worktree + base := d.base + showRendered := d.showRendered + return func() tea.Msg { + text, kind, empty, err := loadFileContent(worktree, base, entry, showRendered) + return diffContentLoadedMsg{ + taskID: taskID, + path: entry.path, + showRendered: showRendered, + text: text, + kind: kind, + isMD: isMarkdown(entry.path), + empty: empty, + err: err, + } + } +} + +func (d *diffViewer) selectedPath() string { + if d == nil || d.selected < 0 || d.selected >= len(d.files) { + return "" + } + return d.files[d.selected].path +} + +// --- layout helpers ------------------------------------------------------- + +// diffTreeWidth returns the width of the file-tree column. +func (m *DetailModel) diffTreeWidth() int { + inner := m.width - 4 + tw := 30 + if max := inner / 3; tw > max { + tw = max + } + if tw < 16 { + tw = 16 + } + if tw > inner-10 { + tw = inner - 10 + } + if tw < 0 { + tw = 0 + } + return tw +} + +// contentViewportWidth returns the viewport width for the content pane, which +// shrinks to leave room for the file tree while the viewer is open. +func (m *DetailModel) contentViewportWidth() int { + base := m.width - 4 + if base < 1 { + base = 1 + } + if m.diff != nil && m.diff.active { + w := base - m.diffTreeWidth() - 1 // 1 col gutter between tree and content + if w < 10 { + w = 10 + } + return w + } + return base +} + +// renderDiffContent renders the content-pane text (right column) that goes into +// the viewport while the viewer is open. +func (m *DetailModel) renderDiffContent() string { + d := m.diff + var b strings.Builder + + mode := "diff" + if d.showRendered { + mode = "file" + } + title := "Changes" + if d.baseLabel != "" { + title = "Diff vs " + d.baseLabel + } + b.WriteString(Bold.Render(title)) + if len(d.files) > 0 && d.selected < len(d.files) { + b.WriteString(Dim.Render(fmt.Sprintf(" — %s [%s]", d.files[d.selected].path, mode))) + } + b.WriteString("\n\n") + + switch { + case d.worktree == "": + b.WriteString(Dim.Render("This task has no worktree yet — nothing to diff.")) + case d.loading: + b.WriteString(Dim.Render("Loading changed files…")) + case d.loadErr != "": + b.WriteString(lipgloss.NewStyle().Foreground(lipgloss.Color("#E06C75")).Render(d.loadErr)) + case len(d.files) == 0: + label := d.baseLabel + if label == "" { + label = "base" + } + b.WriteString(Dim.Render("No changes vs " + label + ".")) + default: + b.WriteString(d.rendered) + } + return b.String() +} + +// renderDiffTree renders the file-tree column (left). It is rendered directly by +// View(); its state is folded into viewSignature so the cache stays correct. +func (m *DetailModel) renderDiffTree(height int) string { + d := m.diff + tw := m.diffTreeWidth() + if tw <= 0 || height <= 0 { + return "" + } + + var lines []string + header := fmt.Sprintf("Changed files (%d)", len(d.files)) + lines = append(lines, Bold.Render(truncate(header, tw))) + lines = append(lines, "") + + if d.loading { + lines = append(lines, Dim.Render("Loading…")) + } else if len(d.files) == 0 { + lines = append(lines, Dim.Render("— none —")) + } else { + // Window the list so the selected entry stays visible. + visible := height - 2 + if visible < 1 { + visible = 1 + } + start := 0 + if d.selected >= visible { + start = d.selected - visible + 1 + } + end := start + visible + if end > len(d.files) { + end = len(d.files) + } + for i := start; i < end; i++ { + lines = append(lines, m.renderTreeRow(d.files[i], i == d.selected, tw)) + } + } + + col := lipgloss.NewStyle().Width(tw).Height(height) + body := strings.Join(lines, "\n") + return col.Render(body) +} + +func (m *DetailModel) renderTreeRow(entry diffFileEntry, selected bool, width int) string { + badge := statusStyle(entry.status).Render(statusGlyph(entry.status)) + name := entry.path + // Show a compact path: keep the trailing segments that fit. + avail := width - 2 // badge + space + if avail < 4 { + avail = 4 + } + name = truncate(name, avail) + row := badge + " " + name + if selected { + sel := lipgloss.NewStyle(). + Background(ColorPrimary). + Foreground(lipgloss.Color("#1A1B26")). + Bold(true). + Width(width) + return sel.Render(truncate(statusGlyph(entry.status)+" "+entry.path, width)) + } + return row +} + +// --- git helpers (run on goroutines) -------------------------------------- + +// loadChangedFiles resolves the base ref and lists the files changed on the task +// branch vs that base (committed, staged, unstaged, and untracked). +func loadChangedFiles(worktree, sourceBranch string) (base, label string, files []diffFileEntry, err error) { + base, label = resolveDiffBase(worktree, sourceBranch) + + seen := map[string]bool{} + // Tracked changes vs the base. + out, derr := gitOutput(worktree, "diff", "--name-status", "--no-renames", base) + if derr != nil { + // Base may be unusable; fall back to working-tree-vs-HEAD. + base, label = "HEAD", "HEAD" + out, derr = gitOutput(worktree, "diff", "--name-status", "--no-renames", base) + if derr != nil { + return base, label, nil, derr + } + } + for _, line := range strings.Split(out, "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + fields := strings.Fields(line) + if len(fields) < 2 { + continue + } + status := string(fields[0][0]) + path := fields[len(fields)-1] + if seen[path] { + continue + } + seen[path] = true + files = append(files, diffFileEntry{path: path, status: status}) + } + + // Untracked files (newly created, not yet added). + if uout, uerr := gitOutput(worktree, "ls-files", "--others", "--exclude-standard"); uerr == nil { + for _, line := range strings.Split(uout, "\n") { + path := strings.TrimSpace(line) + if path == "" || seen[path] { + continue + } + seen[path] = true + files = append(files, diffFileEntry{path: path, status: "?"}) + } + } + + sortDiffFiles(files) + return base, label, files, nil +} + +// loadFileContent loads either the unified diff or the rendered file content for +// a single changed file. +func loadFileContent(worktree, base string, entry diffFileEntry, showRendered bool) (text string, kind diffContentKind, empty bool, err error) { + if showRendered { + // Rendered file: read the current working-tree file. + if entry.status == "D" { + return "", diffKindFile, true, nil + } + data, rerr := os.ReadFile(filepath.Join(worktree, entry.path)) + if rerr != nil { + return "", diffKindFile, false, rerr + } + return clampText(string(data)), diffKindFile, len(data) == 0, nil + } + + // Unified diff. + var out string + if entry.status == "?" { + // Untracked: diff against /dev/null so the whole file shows as added. + // git exits 1 when there's a diff, which is expected here. + out, _ = gitOutput(worktree, "diff", "--no-index", "--", os.DevNull, filepath.Join(worktree, entry.path)) + } else { + out, err = gitOutput(worktree, "diff", base, "--", entry.path) + if err != nil { + return "", diffKindDiff, false, err + } + } + out = clampText(out) + return out, diffKindDiff, strings.TrimSpace(out) == "", nil +} + +// resolveDiffBase finds a good base ref to diff the task branch against and +// returns its merge-base sha plus a human label. +func resolveDiffBase(worktree, sourceBranch string) (base, label string) { + candidates := []string{} + if sourceBranch != "" { + candidates = append(candidates, sourceBranch) + } + if def := defaultBranchName(worktree); def != "" { + candidates = append(candidates, def) + } + candidates = append(candidates, "main", "master") + + tried := map[string]bool{} + for _, c := range candidates { + if c == "" || tried[c] { + continue + } + tried[c] = true + if mb, err := gitOutput(worktree, "merge-base", c, "HEAD"); err == nil { + mb = strings.TrimSpace(mb) + if mb != "" { + return mb, c + } + } + } + // Fallback: compare against HEAD (uncommitted changes only). + return "HEAD", "HEAD" +} + +// defaultBranchName mirrors executor.getDefaultBranch but scoped to the worktree. +func defaultBranchName(worktree string) string { + if out, err := gitOutput(worktree, "symbolic-ref", "refs/remotes/origin/HEAD"); err == nil { + ref := strings.TrimSpace(out) + parts := strings.Split(ref, "/") + if len(parts) > 0 { + return parts[len(parts)-1] + } + } + for _, b := range []string{"main", "master"} { + if err := osExec.Command("git", "-C", worktree, "rev-parse", "--verify", b).Run(); err == nil { + return b + } + } + return "" +} + +func gitOutput(worktree string, args ...string) (string, error) { + full := append([]string{"-C", worktree}, args...) + out, err := osExec.Command("git", full...).Output() + return string(out), err +} + +// --- rendering helpers ---------------------------------------------------- + +// renderViewerMarkdown renders markdown with glamour at the content-pane width. +func (m *DetailModel) renderViewerMarkdown(src string) string { + width := m.contentViewportWidth() + if width < 10 { + width = 10 + } + style := "dark" + if !m.focused { + style = "notty" + } + renderer, err := glamour.NewTermRenderer( + glamour.WithStylePath(style), + glamour.WithWordWrap(width), + ) + if err != nil { + return src + } + out, err := renderer.Render(src) + if err != nil { + return src + } + return strings.TrimSpace(out) +} + +// highlightSource highlights source/diff text with chroma for terminal output. +func highlightSource(source, filename string) string { + lexer := lexers.Match(filename) + if lexer == nil { + lexer = lexers.Analyse(source) + } + if lexer == nil { + lexer = lexers.Fallback + } + lexer = chroma.Coalesce(lexer) + + style := styles.Get("github-dark") + if style == nil { + style = styles.Fallback + } + formatter := formatters.Get("terminal256") + if formatter == nil { + formatter = formatters.Fallback + } + + it, err := lexer.Tokenise(nil, source) + if err != nil { + return source + } + var sb strings.Builder + if err := formatter.Format(&sb, style, it); err != nil { + return source + } + return sb.String() +} + +func isMarkdown(path string) bool { + ext := strings.ToLower(filepath.Ext(path)) + return ext == ".md" || ext == ".markdown" || ext == ".mdown" +} + +func clampText(s string) string { + if len(s) <= maxDiffBytes { + return s + } + return s[:maxDiffBytes] + "\n\n… (truncated — file too large to display in full)" +} + +func truncate(s string, width int) string { + if width <= 0 { + return "" + } + if lipgloss.Width(s) <= width { + return s + } + if width <= 1 { + return "…" + } + // Trim from the left so the most specific path segment stays visible. + runes := []rune(s) + for len(runes) > 0 && lipgloss.Width("…"+string(runes)) > width { + runes = runes[1:] + } + return "…" + string(runes) +} + +func statusGlyph(status string) string { + switch status { + case "A": + return "A" + case "M": + return "M" + case "D": + return "D" + case "R": + return "R" + case "C": + return "C" + case "?": + return "+" + default: + return status + } +} + +func statusStyle(status string) lipgloss.Style { + switch status { + case "A", "?": + return lipgloss.NewStyle().Foreground(lipgloss.Color("#98C379")) // green + case "M": + return lipgloss.NewStyle().Foreground(lipgloss.Color("#E5C07B")) // yellow + case "D": + return lipgloss.NewStyle().Foreground(lipgloss.Color("#E06C75")) // red + default: + return lipgloss.NewStyle().Foreground(ColorMuted) + } +} + +// sortDiffFiles sorts changed files by path for stable display. +func sortDiffFiles(files []diffFileEntry) { + for i := 1; i < len(files); i++ { + for j := i; j > 0 && files[j-1].path > files[j].path; j-- { + files[j-1], files[j] = files[j], files[j-1] + } + } +} diff --git a/internal/ui/diffviewer_test.go b/internal/ui/diffviewer_test.go new file mode 100644 index 00000000..dd70330c --- /dev/null +++ b/internal/ui/diffviewer_test.go @@ -0,0 +1,334 @@ +package ui + +import ( + "os" + osExec "os/exec" + "path/filepath" + "strings" + "testing" + + tea "github.com/charmbracelet/bubbletea" + + "github.com/bborn/workflow/internal/db" +) + +// --- pure helpers --------------------------------------------------------- + +func TestIsMarkdown(t *testing.T) { + cases := map[string]bool{ + "README.md": true, + "docs/guide.MD": true, + "notes.markdown": true, + "x.mdown": true, + "main.go": false, + "Makefile": false, + "a.txt": false, + } + for path, want := range cases { + if got := isMarkdown(path); got != want { + t.Errorf("isMarkdown(%q) = %v, want %v", path, got, want) + } + } +} + +func TestStatusGlyph(t *testing.T) { + cases := map[string]string{"A": "A", "M": "M", "D": "D", "?": "+"} + for status, want := range cases { + if got := statusGlyph(status); got != want { + t.Errorf("statusGlyph(%q) = %q, want %q", status, got, want) + } + } +} + +func TestTruncate(t *testing.T) { + if got := truncate("short", 20); got != "short" { + t.Errorf("truncate kept-as-is failed: %q", got) + } + got := truncate("internal/ui/diffviewer.go", 10) + if got == "" || []rune(got)[0] != '…' { + t.Errorf("truncate should left-trim with ellipsis, got %q", got) + } + if w := lipglossWidth(got); w > 10 { + t.Errorf("truncate width = %d, want <= 10", w) + } + if truncate("anything", 0) != "" { + t.Error("truncate with width 0 should be empty") + } +} + +func TestClampText(t *testing.T) { + small := "hello" + if clampText(small) != small { + t.Error("clampText should not change small text") + } + big := strings.Repeat("x", maxDiffBytes+100) + out := clampText(big) + if len(out) <= maxDiffBytes || !strings.Contains(out, "truncated") { + t.Errorf("clampText should truncate large text with a notice (len=%d)", len(out)) + } +} + +func TestSortDiffFiles(t *testing.T) { + files := []diffFileEntry{ + {path: "z.go"}, {path: "a.go"}, {path: "m/b.go"}, + } + sortDiffFiles(files) + if files[0].path != "a.go" || files[1].path != "m/b.go" || files[2].path != "z.go" { + t.Errorf("sortDiffFiles wrong order: %+v", files) + } +} + +// --- git integration ------------------------------------------------------ + +func git(t *testing.T, dir string, args ...string) { + t.Helper() + full := append([]string{"-C", dir}, args...) + cmd := osExec.Command("git", full...) + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@t", + "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@t", + ) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v failed: %v\n%s", args, err, out) + } +} + +// newTestRepo builds a worktree on a feature branch with a mix of committed, +// uncommitted, and untracked changes vs main, and returns its path. +func newTestRepo(t *testing.T) string { + t.Helper() + dir := t.TempDir() + git(t, dir, "init", "-b", "main") + if err := os.WriteFile(filepath.Join(dir, "base.go"), []byte("package main\n\nfunc main() {}\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "README.md"), []byte("# Title\n\noriginal\n"), 0o644); err != nil { + t.Fatal(err) + } + git(t, dir, "add", ".") + git(t, dir, "commit", "-m", "base") + + git(t, dir, "checkout", "-b", "feature") + // Committed modification. + if err := os.WriteFile(filepath.Join(dir, "base.go"), []byte("package main\n\nfunc main() { println(\"hi\") }\n"), 0o644); err != nil { + t.Fatal(err) + } + git(t, dir, "commit", "-am", "change base") + // Uncommitted modification of a tracked markdown file. + if err := os.WriteFile(filepath.Join(dir, "README.md"), []byte("# Title\n\nupdated body\n"), 0o644); err != nil { + t.Fatal(err) + } + // Untracked new file. + if err := os.WriteFile(filepath.Join(dir, "new.txt"), []byte("brand new\n"), 0o644); err != nil { + t.Fatal(err) + } + return dir +} + +func TestLoadChangedFiles(t *testing.T) { + dir := newTestRepo(t) + base, label, files, err := loadChangedFiles(dir, "") + if err != nil { + t.Fatalf("loadChangedFiles: %v", err) + } + if label != "main" { + t.Errorf("base label = %q, want main", label) + } + if base == "" || base == "HEAD" { + t.Errorf("expected a merge-base sha, got %q", base) + } + got := map[string]string{} + for _, f := range files { + got[f.path] = f.status + } + if got["base.go"] != "M" { + t.Errorf("base.go status = %q, want M", got["base.go"]) + } + if got["README.md"] != "M" { + t.Errorf("README.md status = %q, want M", got["README.md"]) + } + if got["new.txt"] != "?" { + t.Errorf("new.txt status = %q, want ? (untracked)", got["new.txt"]) + } +} + +func TestLoadFileContentDiffAndRendered(t *testing.T) { + dir := newTestRepo(t) + base, _, _, err := loadChangedFiles(dir, "") + if err != nil { + t.Fatal(err) + } + + // Diff mode for a tracked, modified file. + diff, kind, empty, err := loadFileContent(dir, base, diffFileEntry{path: "base.go", status: "M"}, false) + if err != nil || empty || kind != diffKindDiff { + t.Fatalf("diff load: err=%v empty=%v kind=%v", err, empty, kind) + } + if !strings.Contains(diff, "+func main() { println") { + t.Errorf("diff missing the added line:\n%s", diff) + } + + // Diff mode for an untracked file (whole file shows as added). + udiff, _, uempty, err := loadFileContent(dir, base, diffFileEntry{path: "new.txt", status: "?"}, false) + if err != nil { + t.Fatalf("untracked diff: %v", err) + } + if uempty || !strings.Contains(udiff, "brand new") { + t.Errorf("untracked diff should include new content:\n%s", udiff) + } + + // Rendered mode reads the working-tree file. + rendered, kind, empty, err := loadFileContent(dir, base, diffFileEntry{path: "README.md", status: "M"}, true) + if err != nil || empty || kind != diffKindFile { + t.Fatalf("rendered load: err=%v empty=%v kind=%v", err, empty, kind) + } + if !strings.Contains(rendered, "updated body") { + t.Errorf("rendered file should reflect working tree:\n%s", rendered) + } +} + +func TestResolveDiffBaseFallback(t *testing.T) { + // A non-repo dir should fall back to HEAD/HEAD without panicking. + base, label := resolveDiffBase(t.TempDir(), "") + if base != "HEAD" || label != "HEAD" { + t.Errorf("resolveDiffBase fallback = (%q,%q), want (HEAD,HEAD)", base, label) + } +} + +// --- DetailModel integration ---------------------------------------------- + +func newViewerModel(t *testing.T, worktree string) *DetailModel { + t.Helper() + m := &DetailModel{ + task: &db.Task{ID: 1, WorktreePath: worktree, Body: "task body"}, + width: 120, + height: 40, + focused: true, + } + m.initViewport() + return m +} + +func TestContentViewportWidthShrinksWhenViewerActive(t *testing.T) { + m := newViewerModel(t, "") + full := m.contentViewportWidth() + m.diff = &diffViewer{active: true} + narrowed := m.contentViewportWidth() + if narrowed >= full { + t.Errorf("expected content width to shrink when viewer active: full=%d narrowed=%d", full, narrowed) + } + if narrowed < 10 { + t.Errorf("content width should be clamped to >= 10, got %d", narrowed) + } +} + +func TestViewSignatureChangesWithViewerState(t *testing.T) { + m := newViewerModel(t, "") + sigBase := m.viewSignature("h", "help") + + m.diff = &diffViewer{active: true, files: []diffFileEntry{{path: "a"}, {path: "b"}}} + sigActive := m.viewSignature("h", "help") + if sigActive == sigBase { + t.Error("viewSignature should change when the viewer becomes active") + } + + m.diff.selected = 1 + sigSelected := m.viewSignature("h", "help") + if sigSelected == sigActive { + t.Error("viewSignature should change when the selected file changes") + } + + m.diff.showRendered = true + if m.viewSignature("h", "help") == sigSelected { + t.Error("viewSignature should change when toggling rendered mode") + } +} + +func TestOpenAndCloseFileViewer(t *testing.T) { + dir := newTestRepo(t) + m := newViewerModel(t, dir) + + if m.FileViewerActive() { + t.Fatal("viewer should start inactive") + } + cmd := m.OpenFileViewer() + if !m.FileViewerActive() { + t.Fatal("viewer should be active after OpenFileViewer") + } + if cmd == nil { + t.Fatal("OpenFileViewer should return a load command for a worktree task") + } + // renderContent should now produce the viewer pane (loading state). + if !strings.Contains(m.renderContent(), "Changed") && !strings.Contains(m.renderContent(), "Loading") && !strings.Contains(m.renderContent(), "Diff") { + t.Errorf("renderContent should show viewer content, got:\n%s", m.renderContent()) + } + + // Run the load command and feed the result back. + msg := cmd() + loaded, ok := msg.(diffFilesLoadedMsg) + if !ok { + t.Fatalf("expected diffFilesLoadedMsg, got %T", msg) + } + m.HandleDiffFilesLoaded(loaded) + if len(m.diff.files) == 0 { + t.Fatal("expected changed files to be populated") + } + + // Navigate down through the tree. + before := m.diff.selected + handled, _ := m.HandleFileViewerKey(tea.KeyMsg{Type: tea.KeyDown}) + if !handled { + t.Error("down key should be consumed by the viewer") + } + if m.diff.selected == before && len(m.diff.files) > 1 { + t.Error("down key should advance the selection") + } + + // esc closes the viewer. + handled, _ = m.HandleFileViewerKey(tea.KeyMsg{Type: tea.KeyEsc}) + if !handled || m.FileViewerActive() { + t.Error("esc should close the viewer") + } + // Back to normal task content (the "Description" label is always present + // for a task with a body, and is not split by glamour styling). + if !strings.Contains(m.renderContent(), "Description") { + t.Error("closing the viewer should restore task content") + } +} + +func TestHandleDiffContentLoadedRenders(t *testing.T) { + dir := newTestRepo(t) + m := newViewerModel(t, dir) + m.OpenFileViewer() + m.diff.files = []diffFileEntry{{path: "README.md", status: "M"}} + m.diff.selected = 0 + m.diff.showRendered = true + + m.HandleDiffContentLoaded(diffContentLoadedMsg{ + taskID: 1, + path: "README.md", + showRendered: true, + text: "# Heading\n\nbody text\n", + kind: diffKindFile, + isMD: true, + }) + if m.diff.rendered == "" { + t.Fatal("expected rendered markdown content") + } + // glamour styles each word separately, so check the words individually. + if !strings.Contains(m.diff.rendered, "body") || !strings.Contains(m.diff.rendered, "text") { + t.Errorf("rendered markdown should contain the body words:\n%s", m.diff.rendered) + } +} + +func TestFileViewerKeyIgnoredWhenInactive(t *testing.T) { + m := newViewerModel(t, "") + if handled, _ := m.HandleFileViewerKey(tea.KeyMsg{Type: tea.KeyDown}); handled { + t.Error("keys should not be consumed when the viewer is inactive") + } +} + +// lipglossWidth is a tiny wrapper so the helper test does not import lipgloss. +func lipglossWidth(s string) int { + return len([]rune(s)) +} From 9ddad3fc9e4af6a74eb1852b44e2828ac631c61c Mon Sep 17 00:00:00 2001 From: Bruno Bornsztein Date: Wed, 24 Jun 2026 15:34:18 -0500 Subject: [PATCH 2/3] feat(detail): interactive review comments in the diff viewer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turn the read-only diff viewer into an interactive review surface that loops feedback straight back to the task's live agent — no PR round-trip. - A vim-style line cursor in diff mode (j/k); the cursor line is where a comment anchors. File/rendered mode keeps j/k as scroll. - `c` opens an inline comment input (anchored to the current file + the quoted diff line); comments accumulate per task and show a count badge. - `s` composes the collected comments into one structured message and sends it to the executor's tmux pane (m.claudePaneID, since the detail view joins that pane into the UI session). When no agent is running (e.g. a done task) it falls back to copying the review to the clipboard so nothing is lost. All new state (cursor, comments, input, status) is folded into viewSignature() so the View() render cache stays correct, and the comment input is routed like the other live text inputs in app.go (with the viewer's async result messages exempted by type). Adds atotto/clipboard as a direct dep. Tests cover compose/clipboard/cursor/anchor/input flow. Co-Authored-By: Claude Opus 4.8 --- go.mod | 2 +- internal/ui/app.go | 20 +++ internal/ui/detail.go | 41 ++++- internal/ui/diffviewer.go | 314 ++++++++++++++++++++++++++++++++- internal/ui/diffviewer_test.go | 151 ++++++++++++++++ 5 files changed, 519 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 6a4e8679..62e869f4 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.25.0 require ( github.com/alecthomas/chroma/v2 v2.20.0 + github.com/atotto/clipboard v0.1.4 github.com/charmbracelet/bubbles v1.0.0 github.com/charmbracelet/bubbletea v1.3.10 github.com/charmbracelet/glamour v1.0.0 @@ -22,7 +23,6 @@ require ( require ( github.com/anmitsu/go-shlex v0.0.0-20200514113438-38f4b401e2be // indirect - github.com/atotto/clipboard v0.1.4 // indirect github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect github.com/aymerick/douceur v0.2.0 // indirect github.com/catppuccin/go v0.3.0 // indirect diff --git a/internal/ui/app.go b/internal/ui/app.go index 6f185125..a2b67bbd 100644 --- a/internal/ui/app.go +++ b/internal/ui/app.go @@ -787,6 +787,18 @@ func (m *AppModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m.updateDetail(msg) } + // Diff viewer comment input: route all messages (keys + cursor blink) so the + // text input stays live, but exempt the viewer's own async result messages + // so they reach their top-level cases instead of being eaten here. + if m.currentView == ViewDetail && m.detailView != nil && m.detailView.InCommentInput() { + switch msg.(type) { + case diffContentLoadedMsg, diffFilesLoadedMsg, reviewSentMsg: + // fall through to the main switch / default detail routing + default: + return m.updateDetail(msg) + } + } + // Handle filter input mode (needs all message types for text input) if m.currentView == ViewDashboard && m.filterActive { return m.updateFilterMode(msg) @@ -2473,6 +2485,14 @@ func (m *AppModel) updateDetail(msg tea.Msg) (tea.Model, tea.Cmd) { return m, cmd } + // While the diff viewer's comment input is open, route every key into it + // (bypassing the detail-view keybindings) so the user can type freely. + if m.detailView != nil && m.detailView.InCommentInput() { + var cmd tea.Cmd + m.detailView, cmd = m.detailView.UpdateCommentInput(msg) + return m, cmd + } + // Handle key messages keyMsg, ok := msg.(tea.KeyMsg) if !ok { diff --git a/internal/ui/detail.go b/internal/ui/detail.go index 5cebd0ee..8ed79c47 100644 --- a/internal/ui/detail.go +++ b/internal/ui/detail.go @@ -870,6 +870,11 @@ func (m *DetailModel) Update(msg tea.Msg) (*DetailModel, tea.Cmd) { // Selected file's diff/rendered content loaded. m.HandleDiffContentLoaded(msg) return m, nil + + case reviewSentMsg: + // Review comments delivered to the executor (or clipboard). + m.HandleReviewSent(msg) + return m, nil } // Pass all messages to viewport for scrolling support @@ -2489,6 +2494,13 @@ func (m *DetailModel) viewSignature(header, help string) uint64 { h.int(len(m.diff.files)) h.boolean(m.diff.showRendered) h.str(m.diff.loadErr) + // Interactive review state: the line cursor, comment input, and status + // all affect the rendered content/footer, so fold them in. + h.int(m.diff.cursor) + h.int(len(m.diff.comments)) + h.boolean(m.diff.commenting) + h.str(m.diff.input.Value()) + h.str(m.diff.statusMsg) } h.str(header) h.str(help) @@ -3071,10 +3083,25 @@ func (m *DetailModel) renderHelp() string { // File/diff viewer has its own, focused help line. if m.diff != nil && m.diff.active { + // While typing a comment, the footer is the input field. + if m.diff.commenting { + prompt := HelpKey.Render("comment") + " " + m.diff.input.View() + + " " + HelpKey.Render("enter") + " " + HelpDesc.Render("save") + + " " + HelpKey.Render("esc") + " " + HelpDesc.Render("cancel") + return HelpBar.Render(prompt) + } + noFiles := len(m.diff.files) == 0 + cursorMode := m.diff.cursorActive() + scrollDesc := "scroll" + if cursorMode { + scrollDesc = "line" + } viewerKeys := []helpKey{ - {IconArrowUp() + "/" + IconArrowDown(), "file", len(m.diff.files) == 0}, - {"j/k/wheel", "scroll", false}, - {"tab", "diff/rendered", len(m.diff.files) == 0}, + {IconArrowUp() + "/" + IconArrowDown(), "file", noFiles}, + {"j/k", scrollDesc, false}, + {"tab", "diff/rendered", noFiles}, + {"c", "comment", noFiles}, + {"s", "send", len(m.diff.comments) == 0}, {"esc", "close", false}, } var vh string @@ -3089,6 +3116,14 @@ func (m *DetailModel) renderHelp() string { vh += HelpKey.Render(k.key) + " " + HelpDesc.Render(k.desc) } } + // Transient status (sent / copied / error) replaces the tail of the bar. + if m.diff.statusMsg != "" { + col := lipgloss.Color("#98C379") + if m.diff.statusIsErr { + col = lipgloss.Color("#E06C75") + } + vh += " " + lipgloss.NewStyle().Foreground(col).Render(m.diff.statusMsg) + } return HelpBar.Render(vh) } diff --git a/internal/ui/diffviewer.go b/internal/ui/diffviewer.go index bb74e56d..2c47d5c6 100644 --- a/internal/ui/diffviewer.go +++ b/internal/ui/diffviewer.go @@ -20,6 +20,8 @@ import ( "path/filepath" "strings" + "github.com/atotto/clipboard" + "github.com/charmbracelet/bubbles/textinput" tea "github.com/charmbracelet/bubbletea" "github.com/charmbracelet/glamour" "github.com/charmbracelet/lipgloss" @@ -59,9 +61,27 @@ type diffViewer struct { // Content pane state for the currently selected file. contentLoading bool - contentPath string // path the rendered content belongs to - contentMode bool // showRendered value the content was rendered for - rendered string // final, ready-to-display content string + contentPath string // path the rendered content belongs to + contentMode bool // showRendered value the content was rendered for + rendered string // final, ready-to-display content string + rawLines []string // raw (unhighlighted) content lines; set in diff mode for the line cursor + cursor int // cursor line index into rawLines (diff mode only) + + // Interactive review: comments the user attaches to the diff, to be sent to + // the task's live executor (or copied to the clipboard when none is running). + comments []reviewComment + commenting bool // true while the comment text input is open + input textinput.Model // comment text input + statusMsg string // transient status line (e.g. "Sent 3 comments") + statusIsErr bool +} + +// reviewComment is one piece of review feedback anchored to a file (and, in diff +// mode, a specific line quoted from the diff). +type reviewComment struct { + file string + line string // quoted diff line the comment anchors to ("" for file-level) + body string } // --- messages ------------------------------------------------------------- @@ -86,6 +106,13 @@ type diffContentLoadedMsg struct { empty bool // no changes / nothing to show } +type reviewSentMsg struct { + taskID int64 + count int + viaClipboard bool + err error +} + type diffContentKind int const ( @@ -118,8 +145,14 @@ func (m *DetailModel) OpenFileViewer() tea.Cmd { d.selected = 0 d.showRendered = false d.rendered = "" + d.rawLines = nil + d.cursor = 0 d.contentPath = "" d.contentLoading = false + d.commenting = false + d.statusMsg = "" + // Note: d.comments is intentionally preserved so reopening the viewer keeps + // any unsent review feedback. // Narrow the viewport to the content column and reset scroll. if m.ready { @@ -185,16 +218,249 @@ func (m *DetailModel) HandleFileViewerKey(msg tea.KeyMsg) (bool, tea.Cmd) { return true, m.loadSelectedFileContent() } return true, nil - case "tab", "enter", " ": + case "tab": if len(d.files) > 0 { d.showRendered = !d.showRendered return true, m.loadSelectedFileContent() } return true, nil + case "j": + // Move the line cursor down in diff mode; otherwise fall through to scroll. + if d.cursorActive() { + m.moveCursor(1) + return true, nil + } + return false, nil + case "k": + if d.cursorActive() { + m.moveCursor(-1) + return true, nil + } + return false, nil + case "c": + // Start a review comment on the current file/line. + if len(d.files) > 0 { + return true, m.startComment() + } + return true, nil + case "s": + // Send the collected review to the executor (or clipboard). + if len(d.comments) > 0 { + return true, m.sendReviewCmd() + } + d.statusMsg = "No comments yet — press c to add one" + d.statusIsErr = true + m.setViewportContent() + return true, nil } return false, nil } +// moveCursor moves the diff line cursor and keeps it visible in the viewport. +func (m *DetailModel) moveCursor(delta int) { + d := m.diff + n := len(d.rawLines) + if n == 0 { + return + } + d.cursor += delta + if d.cursor < 0 { + d.cursor = 0 + } + if d.cursor >= n { + d.cursor = n - 1 + } + // Keep the cursor row (offset by the 2-line content header) on screen. + row := d.cursor + diffHeaderLines + if row < m.viewport.YOffset { + m.viewport.SetYOffset(row) + } else if row >= m.viewport.YOffset+m.viewport.Height { + m.viewport.SetYOffset(row - m.viewport.Height + 1) + } + m.setViewportContent() +} + +// diffHeaderLines is the number of lines renderDiffContent prepends before the +// actual diff/file body (title + blank). Used for cursor scroll math. +const diffHeaderLines = 2 + +// --- interactive review comments ------------------------------------------ + +// InCommentInput reports whether the comment text input is currently open. +func (m *DetailModel) InCommentInput() bool { + return m.diff != nil && m.diff.active && m.diff.commenting +} + +// startComment opens the comment text input for the current file/line. +func (m *DetailModel) startComment() tea.Cmd { + d := m.diff + ti := textinput.New() + ti.Placeholder = "comment for the agent…" + ti.Prompt = "› " + ti.CharLimit = 500 + ti.Width = m.contentViewportWidth() - 6 + ti.Focus() + d.input = ti + d.commenting = true + d.statusMsg = "" + d.statusIsErr = false + m.setViewportContent() + return textinput.Blink +} + +// UpdateCommentInput feeds a message to the open comment input. Enter saves the +// comment, Esc cancels; everything else edits the text. +func (m *DetailModel) UpdateCommentInput(msg tea.Msg) (*DetailModel, tea.Cmd) { + d := m.diff + if d == nil || !d.commenting { + return m, nil + } + if key, ok := msg.(tea.KeyMsg); ok { + switch key.String() { + case "esc": + d.commenting = false + d.input.Blur() + m.setViewportContent() + return m, nil + case "enter": + body := strings.TrimSpace(d.input.Value()) + if body != "" { + d.comments = append(d.comments, reviewComment{ + file: d.selectedPath(), + line: d.cursorAnchor(), + body: body, + }) + d.statusMsg = fmt.Sprintf("Added comment (%d pending)", len(d.comments)) + d.statusIsErr = false + } + d.commenting = false + d.input.Blur() + m.setViewportContent() + return m, nil + } + } + var cmd tea.Cmd + d.input, cmd = d.input.Update(msg) + m.setViewportContent() + return m, cmd +} + +// cursorAnchor returns the trimmed diff line the cursor is on (without the +// leading +/-/space marker), or "" for file-level comments. +func (d *diffViewer) cursorAnchor() string { + if !d.cursorActive() || d.cursor < 0 || d.cursor >= len(d.rawLines) { + return "" + } + line := d.rawLines[d.cursor] + if len(line) > 0 && (line[0] == '+' || line[0] == '-' || line[0] == ' ') { + line = line[1:] + } + return strings.TrimSpace(line) +} + +// sendReviewCmd composes the collected comments and delivers them to the task's +// live executor pane, or copies them to the clipboard when no agent is running. +func (m *DetailModel) sendReviewCmd() tea.Cmd { + if m.task == nil || m.diff == nil || len(m.diff.comments) == 0 { + return nil + } + taskID := m.task.ID + pane := m.claudePaneID + count := len(m.diff.comments) + line := composeReviewLine(m.task.BranchName, m.diff.comments) + block := composeReviewBlock(m.task.BranchName, m.diff.comments) + return func() tea.Msg { + if pane != "" { + if err := sendLiteralToPane(pane, line); err != nil { + return reviewSentMsg{taskID: taskID, count: count, err: err} + } + return reviewSentMsg{taskID: taskID, count: count} + } + // No live executor — fall back to the clipboard so the review isn't lost. + if err := clipboard.WriteAll(block); err != nil { + return reviewSentMsg{taskID: taskID, count: count, err: err} + } + return reviewSentMsg{taskID: taskID, count: count, viaClipboard: true} + } +} + +// HandleReviewSent applies the result of a send attempt. +func (m *DetailModel) HandleReviewSent(msg reviewSentMsg) { + if m.diff == nil || m.task == nil || msg.taskID != m.task.ID { + return + } + d := m.diff + switch { + case msg.err != nil: + d.statusMsg = "Send failed: " + msg.err.Error() + d.statusIsErr = true + case msg.viaClipboard: + d.comments = nil + d.statusMsg = fmt.Sprintf("No live executor — copied %d comments to clipboard", msg.count) + d.statusIsErr = false + default: + d.comments = nil + d.statusMsg = fmt.Sprintf("Sent %d comments to the agent", msg.count) + d.statusIsErr = false + } + m.setViewportContent() +} + +// sendLiteralToPane sends literal text + Enter to a specific tmux pane id. We +// target the persisted pane id (not the daemon session) because the detail view +// joins the executor pane into the UI session. +func sendLiteralToPane(paneID, text string) error { + if err := osExec.Command("tmux", "send-keys", "-t", paneID, "-l", text).Run(); err != nil { + return err + } + return osExec.Command("tmux", "send-keys", "-t", paneID, "Enter").Run() +} + +// composeReviewLine builds a single-line review message (safe to send to a TUI +// agent without embedded newlines triggering an early submit). +func composeReviewLine(branch string, comments []reviewComment) string { + var b strings.Builder + b.WriteString("Code review") + if branch != "" { + b.WriteString(" on " + branch) + } + b.WriteString(fmt.Sprintf(" (%d comments): ", len(comments))) + for i, c := range comments { + if i > 0 { + b.WriteString(" ") + } + b.WriteString(fmt.Sprintf("[%d] %s", i+1, c.file)) + if c.line != "" { + b.WriteString(fmt.Sprintf(" @ `%s`", c.line)) + } + b.WriteString(": " + c.body) + if !strings.HasSuffix(strings.TrimSpace(c.body), ".") { + b.WriteString(".") + } + } + b.WriteString(" Please address these in the worktree.") + return b.String() +} + +// composeReviewBlock builds a readable multi-line version for the clipboard. +func composeReviewBlock(branch string, comments []reviewComment) string { + var b strings.Builder + b.WriteString("Code review") + if branch != "" { + b.WriteString(" on " + branch) + } + b.WriteString(":\n\n") + for i, c := range comments { + b.WriteString(fmt.Sprintf("%d. %s", i+1, c.file)) + if c.line != "" { + b.WriteString(fmt.Sprintf("\n > %s", c.line)) + } + b.WriteString("\n " + c.body + "\n\n") + } + b.WriteString("Please address these in the worktree.") + return b.String() +} + // HandleDiffFilesLoaded applies an async file-list load result. func (m *DetailModel) HandleDiffFilesLoaded(msg diffFilesLoadedMsg) tea.Cmd { if m.diff == nil || !m.diff.active || m.task == nil || msg.taskID != m.task.ID { @@ -232,6 +498,8 @@ func (m *DetailModel) HandleDiffContentLoaded(msg diffContentLoadedMsg) { d.contentLoading = false d.contentPath = msg.path d.contentMode = msg.showRendered + d.rawLines = nil + d.cursor = 0 switch { case msg.err != nil: d.rendered = lipgloss.NewStyle().Foreground(lipgloss.Color("#E06C75")). @@ -242,13 +510,20 @@ func (m *DetailModel) HandleDiffContentLoaded(msg diffContentLoadedMsg) { d.rendered = m.renderViewerMarkdown(msg.text) case msg.kind == diffKindFile: d.rendered = highlightSource(msg.text, msg.path) - default: // diff + default: // diff — keep the raw lines so the line cursor can anchor comments d.rendered = highlightSource(msg.text, "diff.diff") + d.rawLines = strings.Split(strings.TrimRight(msg.text, "\n"), "\n") } m.viewport.GotoTop() m.setViewportContent() } +// cursorActive reports whether the line cursor (and line-anchored comments) apply +// to the current content — i.e. we're viewing a unified diff with raw lines. +func (d *diffViewer) cursorActive() bool { + return !d.showRendered && len(d.rawLines) > 0 +} + // loadSelectedFileContent kicks off async loading of the selected file's content // (diff or rendered file) and shows a loading placeholder immediately. func (m *DetailModel) loadSelectedFileContent() tea.Cmd { @@ -343,10 +618,15 @@ func (m *DetailModel) renderDiffContent() string { if d.baseLabel != "" { title = "Diff vs " + d.baseLabel } + // Keep the header exactly diffHeaderLines tall so the line-cursor scroll math + // stays correct: a single title line + one blank line. b.WriteString(Bold.Render(title)) if len(d.files) > 0 && d.selected < len(d.files) { b.WriteString(Dim.Render(fmt.Sprintf(" — %s [%s]", d.files[d.selected].path, mode))) } + if n := len(d.comments); n > 0 { + b.WriteString(lipgloss.NewStyle().Foreground(ColorPrimary).Render(fmt.Sprintf(" · %d comment%s", n, plural(n)))) + } b.WriteString("\n\n") switch { @@ -362,12 +642,36 @@ func (m *DetailModel) renderDiffContent() string { label = "base" } b.WriteString(Dim.Render("No changes vs " + label + ".")) + case d.cursorActive(): + b.WriteString(markCursorLine(d.rendered, d.cursor)) default: b.WriteString(d.rendered) } return b.String() } +// markCursorLine prefixes each line of rendered content with a gutter, drawing a +// bar on the cursor line so the user can see which line a comment will anchor to. +func markCursorLine(rendered string, cursor int) string { + lines := strings.Split(rendered, "\n") + bar := lipgloss.NewStyle().Foreground(ColorPrimary).Bold(true).Render("▌") + for i := range lines { + if i == cursor { + lines[i] = bar + " " + lines[i] + } else { + lines[i] = " " + lines[i] + } + } + return strings.Join(lines, "\n") +} + +func plural(n int) string { + if n == 1 { + return "" + } + return "s" +} + // renderDiffTree renders the file-tree column (left). It is rendered directly by // View(); its state is folded into viewSignature so the cache stays correct. func (m *DetailModel) renderDiffTree(height int) string { diff --git a/internal/ui/diffviewer_test.go b/internal/ui/diffviewer_test.go index dd70330c..774a4bfe 100644 --- a/internal/ui/diffviewer_test.go +++ b/internal/ui/diffviewer_test.go @@ -328,6 +328,157 @@ func TestFileViewerKeyIgnoredWhenInactive(t *testing.T) { } } +// --- interactive review comments ------------------------------------------ + +func TestComposeReviewLineSingleLine(t *testing.T) { + comments := []reviewComment{ + {file: "server.go", line: "mux.HandleFunc(\"/healthz\", s.health)", body: "guard against nil mux"}, + {file: "README.md", body: "mention the timeout flag"}, + } + out := composeReviewLine("feature/add-health-check", comments) + if strings.Contains(out, "\n") { + t.Errorf("review line must be single-line (no newlines), got:\n%q", out) + } + for _, want := range []string{"feature/add-health-check", "(2 comments)", "server.go", "README.md", "guard against nil mux", "mention the timeout flag", "Please address"} { + if !strings.Contains(out, want) { + t.Errorf("review line missing %q:\n%s", want, out) + } + } +} + +func TestComposeReviewBlockMultiLine(t *testing.T) { + out := composeReviewBlock("br", []reviewComment{{file: "a.go", body: "x"}}) + if !strings.Contains(out, "\n") { + t.Error("review block should be multi-line") + } + if !strings.Contains(out, "1. a.go") { + t.Errorf("review block should number comments:\n%s", out) + } +} + +func TestCursorMovementAndAnchor(t *testing.T) { + dir := newTestRepo(t) + m := newViewerModel(t, dir) + m.OpenFileViewer() + // Simulate a loaded diff with raw lines. + m.diff.files = []diffFileEntry{{path: "server.go", status: "M"}} + m.diff.selected = 0 + m.diff.showRendered = false + m.diff.rendered = "@@ -1 +1 @@\n-old line\n+new line" + m.diff.rawLines = []string{"@@ -1 +1 @@", "-old line", "+new line"} + m.diff.cursor = 0 + + if !m.diff.cursorActive() { + t.Fatal("cursor should be active in diff mode with raw lines") + } + // j moves the cursor down (consumed), k moves up. + if handled, _ := m.HandleFileViewerKey(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("j")}); !handled { + t.Error("j should be consumed as cursor movement in diff mode") + } + if m.diff.cursor != 1 { + t.Errorf("cursor = %d, want 1 after j", m.diff.cursor) + } + // Anchor strips the +/- marker. + if got := m.diff.cursorAnchor(); got != "old line" { + t.Errorf("cursorAnchor = %q, want %q", got, "old line") + } + m.HandleFileViewerKey(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("j")}) + if got := m.diff.cursorAnchor(); got != "new line" { + t.Errorf("cursorAnchor = %q, want %q", got, "new line") + } +} + +func TestCommentInputFlow(t *testing.T) { + dir := newTestRepo(t) + m := newViewerModel(t, dir) + m.OpenFileViewer() + m.diff.files = []diffFileEntry{{path: "server.go", status: "M"}} + m.diff.selected = 0 + m.diff.rawLines = []string{"+added line"} + m.diff.cursor = 0 + + // Open the comment input via the viewer key. + handled, _ := m.HandleFileViewerKey(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("c")}) + if !handled || !m.InCommentInput() { + t.Fatal("c should open the comment input") + } + // Type some text, then Enter to save. + m.UpdateCommentInput(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("needs a test")}) + m.UpdateCommentInput(tea.KeyMsg{Type: tea.KeyEnter}) + if m.InCommentInput() { + t.Error("enter should close the comment input") + } + if len(m.diff.comments) != 1 { + t.Fatalf("expected 1 comment, got %d", len(m.diff.comments)) + } + c := m.diff.comments[0] + if c.file != "server.go" || c.body != "needs a test" || c.line != "added line" { + t.Errorf("unexpected comment: %+v", c) + } +} + +func TestCommentInputCancel(t *testing.T) { + dir := newTestRepo(t) + m := newViewerModel(t, dir) + m.OpenFileViewer() + m.diff.files = []diffFileEntry{{path: "a", status: "M"}} + m.HandleFileViewerKey(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("c")}) + m.UpdateCommentInput(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("draft")}) + m.UpdateCommentInput(tea.KeyMsg{Type: tea.KeyEsc}) + if m.InCommentInput() { + t.Error("esc should close the input") + } + if len(m.diff.comments) != 0 { + t.Error("esc should discard the in-progress comment") + } +} + +func TestSendReviewViaClipboardWhenNoExecutor(t *testing.T) { + dir := newTestRepo(t) + m := newViewerModel(t, dir) + m.OpenFileViewer() + m.claudePaneID = "" // no live executor + m.diff.comments = []reviewComment{{file: "a.go", body: "fix it"}} + + cmd := m.sendReviewCmd() + if cmd == nil { + t.Fatal("sendReviewCmd should return a command when comments exist") + } + msg, ok := cmd().(reviewSentMsg) + if !ok { + t.Fatalf("expected reviewSentMsg, got %T", msg) + } + // Clipboard may be unavailable in headless CI; accept either a clean + // clipboard send or a surfaced error, but never a pane send. + if msg.err == nil && !msg.viaClipboard { + t.Error("with no executor, a successful send must go via clipboard") + } + m.HandleReviewSent(msg) + if msg.err == nil && len(m.diff.comments) != 0 { + t.Error("a successful send should clear pending comments") + } + if m.diff.statusMsg == "" { + t.Error("send should set a status message") + } +} + +func TestViewSignatureChangesWithReviewState(t *testing.T) { + m := newViewerModel(t, "") + m.diff = &diffViewer{active: true, files: []diffFileEntry{{path: "a"}}, rawLines: []string{"x", "y"}} + base := m.viewSignature("h", "help") + + m.diff.cursor = 1 + if m.viewSignature("h", "help") == base { + t.Error("signature should change when the line cursor moves") + } + sig2 := m.viewSignature("h", "help") + + m.diff.comments = append(m.diff.comments, reviewComment{file: "a", body: "c"}) + if m.viewSignature("h", "help") == sig2 { + t.Error("signature should change when a comment is added") + } +} + // lipglossWidth is a tiny wrapper so the helper test does not import lipgloss. func lipglossWidth(s string) int { return len([]rune(s)) From 1436e73a254f07a24a0507bb5e25c0dbf8e31879 Mon Sep 17 00:00:00 2001 From: Bruno Bornsztein Date: Thu, 25 Jun 2026 06:20:10 -0500 Subject: [PATCH 3/3] test(parity): mark ViewDiff as TUI-only in parity-ignore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new `v` diff/file review viewer is rendered inline in the TUI pane (chroma + glamour) and has no native GUI/web-API equivalent, so the GUI-covers-TUI parity test must treat it as intentionally TUI-only — mirroring the existing SpotlightSync entry. Co-Authored-By: Claude Opus 4.8 --- parity-ignore.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/parity-ignore.json b/parity-ignore.json index 8406d11a..314f885d 100644 --- a/parity-ignore.json +++ b/parity-ignore.json @@ -1,3 +1,4 @@ { - "SpotlightSync": "macOS Spotlight metadata export is a local-indexing concern; intentionally TUI/CLI-only." + "SpotlightSync": "macOS Spotlight metadata export is a local-indexing concern; intentionally TUI/CLI-only.", + "ViewDiff": "Read-only git diff/file review viewer rendered inline in the TUI pane (chroma syntax highlighting + glamour markdown); no native GUI/web-API equivalent yet, so intentionally TUI-only for now." }