From 4b75172bb114fc21d9d213f037ee009f28033361 Mon Sep 17 00:00:00 2001 From: Michael Rose Date: Tue, 24 Mar 2026 06:40:07 +0100 Subject: [PATCH 1/4] add initial plan --- .../specs/2026-03-24-github-pr-review/PLAN.md | 228 ++++++++++++++++++ 1 file changed, 228 insertions(+) create mode 100644 docs/specs/2026-03-24-github-pr-review/PLAN.md diff --git a/docs/specs/2026-03-24-github-pr-review/PLAN.md b/docs/specs/2026-03-24-github-pr-review/PLAN.md new file mode 100644 index 0000000..a953853 --- /dev/null +++ b/docs/specs/2026-03-24-github-pr-review/PLAN.md @@ -0,0 +1,228 @@ +# GitHub PR Review Plan for diffman + +## Why this plan + +`diffman` already has strong local-review primitives (file list, parsed diff rows, inline comments, stale detection, export). The fastest path to “actually review GitHub PRs” is to **reuse those primitives** and swap the data source from local git working tree to GitHub PR data. + +--- + +## Current architecture (as-is) + +### Core flow today + +1. `app.NewModel()` discovers repo and initializes services (`statusSvc`, `diffSvc`) and local comment store. +2. `loadFilesCmd()` loads changed files from `git status` via `internal/git/status.go`. +3. Selecting a file triggers `loadDiffCmd(path)` which calls `diffSvc.Diff(...)` from `internal/git/diff.go`. +4. Raw unified diff is parsed by `diffview.ParseUnifiedDiff` into `[]DiffRow`. +5. `diffview.RenderSplitWithLayoutComments` renders side-by-side panes with inline comment blocks. +6. Comments are stored locally in `.git/.diffman/comments.json` (`internal/comments/store.go`). +7. `buildCommentStaleMap(...)` re-validates anchors against current parsed diff rows. + +### Strong reuse points + +- Parser/rendering stack is already PR-ready for unified diffs: + - `internal/diffview/parse.go` + - `internal/diffview/render.go` +- App async command/message pattern is already established: + - `loadFilesCmd`, `loadDiffCmd`, `*_loadedMsg` in `internal/app/model.go` +- Existing comment anchor model (`path + side + line`) aligns with GitHub inline comments. + +### Gaps for GitHub PR reviews + +- No GitHub client/integration package exists. +- No PR session state in `Model` (repo, PR number, base/head SHAs, review state). +- No submission path from local comments to GitHub review comments. +- No CLI args for PR mode (`cmd/diffman/main.go` is currently zero-arg). + +--- + +## Implementation options + +## Option A (recommended): `gh` CLI-backed integration + +Use `gh` for auth and API access (no embedded OAuth/token handling in diffman). + +**Pros** +- Lowest implementation risk and fastest delivery. +- Auth/session reuse from users’ existing `gh auth login`. +- Fits existing `util.Run(...)` command-execution pattern. + +**Cons** +- Runtime dependency on `gh`. +- Need robust parsing/handling for `gh` output and errors. + +## Option B: Native GitHub REST client in Go + +**Pros** +- Full control, fewer external runtime dependencies. + +**Cons** +- Larger scope (auth, token discovery, API plumbing, retries/rate limits). +- Slower to first usable PR-review flow. + +**Decision**: Start with **Option A**. Keep interfaces clean so Option B can replace internals later. + +--- + +## Proposed design (minimal, incremental) + +## Phase 1 — Read-only PR browsing in diffman + +Goal: open a PR and review its diffs in existing UI (no submission yet). + +### 1) Introduce PR review source package + +Add new package, e.g. `internal/githubpr`: + +- `type Service interface { ... }` +- `NewService()` constructor +- Methods (minimum): + - `ResolvePR(ctx, cwd, input) (PRContext, error)` // parse number/url and infer repo + - `ListFiles(ctx, prCtx) ([]git.FileItem, error)` // adapt to existing file list UI + - `Diff(ctx, prCtx, path) (string, error)` // unified diff per file + +Keep API style consistent with existing `internal/git` service interfaces. + +### 2) Add app PR mode state + +In `internal/app/model.go`, add a small PR session struct/state: + +- mode: local vs PR +- PR metadata (repo, number, title, base/head, head SHA) + +Wire alternate loaders: + +- local mode → existing `statusSvc` / `diffSvc` +- PR mode → `githubpr.Service` + +Keep downstream parser/render/comment code unchanged. + +### 3) Add startup entry for PR mode + +In `cmd/diffman/main.go`: + +- Add minimal arg parsing: + - `diffman` (existing local mode) + - `diffman --pr ` (PR mode) + +Pass options into `app.NewModel(...)` (constructor update required). + +### 4) UX updates + +- Show PR context in pane title/footer (e.g., `PR #123 owner/repo`). +- Keep current keybindings for navigation/comment drafting. + +Deliverable: user can browse GitHub PR diffs with existing diff/comment UI. + +--- + +## Phase 2 — Submit review comments to GitHub + +Goal: turn local draft comments into real GitHub PR review comments. + +### 1) Add submission API in `internal/githubpr` + +Minimum methods: + +- `SubmitLineComment(ctx, prCtx, draftComment)` +- `SubmitReview(ctx, prCtx, body, event, draftComments)` (optional in v1, preferred) + +Map existing `comments.Comment` (`path`, `side`, `line`, `body`) to GitHub PR review comment payload using PR `head_sha`. + +### 2) Add app command/action for submit + +Add a keybinding/action in `internal/app/keymap.go` + `Model.Update`: + +- Submit all non-stale draft comments to GitHub +- Surface success/failure via existing alert dock + +### 3) Draft lifecycle rules + +Pick one explicit rule for v1: + +- either clear submitted local drafts, +- or mark as submitted (requires model/store extension). + +Recommendation for v1: **clear on successful submit** (smallest scope), with confirmation prompt. + +Deliverable: user can leave comments in diffman and publish them to PR. + +--- + +## Phase 3 — Review events and thread sync (follow-up) + +Goal: complete PR review workflow parity. + +- Submit `APPROVE` / `REQUEST_CHANGES` / `COMMENT` review events. +- Load existing PR review threads and show inline/thread markers. +- Resolve/unresolve thread support (if desired). + +This phase is intentionally separate to keep first deliverable tight. + +--- + +## File-level change plan + +- `cmd/diffman/main.go` + - add `--pr` option parsing and model initialization wiring. + +- `internal/app/model.go` + - add PR mode/session fields + - add PR-aware load files/diff commands using existing msg/cmd patterns + - add submit-review command handling in later phase + +- `internal/app/keymap.go` + - add keybinding(s) for submit/review actions + +- `internal/comments/model.go` + `store.go` (optional) + - only if we choose “mark submitted” rather than “clear after submit” + +- `internal/githubpr/*` (new) + - service interface + `gh` adapter implementation + - typed models for PR context and submission payloads + +--- + +## Test strategy + +1. **Unit tests (new package)** + - parse/normalize PR input (number/url) + - map GitHub file list → `[]git.FileItem` + - diff/submission command construction and error mapping + +2. **App tests** + - PR mode loader routing (file list + diff) + - submission action success/error alert behavior + - no regressions in local mode + +3. **Existing suites** + - keep parser/render tests unchanged; they are core safety net + +Validation command set: + +- `make test` +- `make lint` + +--- + +## Risks and mitigations + +- **GitHub API/CLI output variance** + - Mitigate with strict parsing + fallback errors surfaced in alert dock. + +- **Diff/line anchor mismatch when PR updates** + - Reuse stale-comment detection and require fresh PR reload before submit if needed. + +- **Scope creep into full GitHub client** + - Keep v1 on `gh` adapter behind interface; defer native client. + +--- + +## Suggested milestone sequence + +1. PR read-only mode (open PR, list files, view diffs). +2. Submit draft comments as PR comments. +3. Add approve/request-changes event support. +4. Thread sync/resolve UX. + +This gives an early, useful end-to-end path while preserving diffman’s current architecture and minimizing churn. From 95a49f81d3bac9f6b986cd1cd4648f831e67e2cc Mon Sep 17 00:00:00 2001 From: Michael Rose Date: Tue, 24 Mar 2026 07:33:34 +0100 Subject: [PATCH 2/4] initial implementation to view diff --- cmd/diffman/main.go | 7 +- internal/app/model.go | 215 ++++++++++++++++++--- internal/app/model_pr_cache_test.go | 105 +++++++++++ internal/githubpr/gh.go | 283 ++++++++++++++++++++++++++++ internal/githubpr/gh_test.go | 86 +++++++++ internal/githubpr/service.go | 28 +++ 6 files changed, 698 insertions(+), 26 deletions(-) create mode 100644 internal/app/model_pr_cache_test.go create mode 100644 internal/githubpr/gh.go create mode 100644 internal/githubpr/gh_test.go create mode 100644 internal/githubpr/service.go diff --git a/cmd/diffman/main.go b/cmd/diffman/main.go index ee72593..dfbb116 100644 --- a/cmd/diffman/main.go +++ b/cmd/diffman/main.go @@ -1,6 +1,7 @@ package main import ( + "flag" "fmt" "os" @@ -10,7 +11,11 @@ import ( ) func main() { - model, err := app.NewModel() + var pr string + flag.StringVar(&pr, "pr", "", "GitHub pull request number or URL") + flag.Parse() + + model, err := app.NewModelWithOptions(app.Options{PR: pr}) if err != nil { fmt.Fprintf(os.Stderr, "failed to initialize app: %v\n", err) os.Exit(1) diff --git a/internal/app/model.go b/internal/app/model.go index 9f0f667..cd36bb2 100644 --- a/internal/app/model.go +++ b/internal/app/model.go @@ -21,6 +21,7 @@ import ( "diffman/internal/config" "diffman/internal/diffview" gitint "diffman/internal/git" + "diffman/internal/githubpr" ) type focusPane int @@ -39,6 +40,22 @@ const ( diffPaneModeNewOnly ) +type reviewMode int + +const ( + reviewModeLocal reviewMode = iota + reviewModePR +) + +type Options struct { + PR string +} + +type prDiffCacheEntry struct { + rows []diffview.DiffRow + empty bool +} + const ( filePaneWidthDefault = 40 filePaneWidthWide = 120 @@ -81,12 +98,16 @@ type commentAnchor struct { // Model is the Bubble Tea state container for the app. type Model struct { - keys KeyMap - focus focusPane - cwd string - diffMode gitint.DiffMode - statusSvc gitint.StatusService - diffSvc gitint.DiffService + keys KeyMap + focus focusPane + cwd string + diffMode gitint.DiffMode + reviewMode reviewMode + statusSvc gitint.StatusService + diffSvc gitint.DiffService + prSvc githubpr.Service + prCtx *githubpr.Context + prDiffs map[string]prDiffCacheEntry width int height int @@ -136,8 +157,11 @@ type Model struct { err error } -// Test func NewModel() (Model, error) { + return NewModelWithOptions(Options{}) +} + +func NewModelWithOptions(opts Options) (Model, error) { cwd, err := os.Getwd() if err != nil { return Model{}, err @@ -171,13 +195,30 @@ func NewModel() (Model, error) { commentInput.Cursor.Style = lipgloss.NewStyle().Foreground(lipgloss.Color("51")) commentInput.PlaceholderStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("244")) + prSvc := githubpr.NewService() + var prCtx *githubpr.Context + prDiffs := make(map[string]prDiffCacheEntry) + mode := reviewModeLocal + if strings.TrimSpace(opts.PR) != "" { + ctx, err := prSvc.ResolvePR(context.Background(), repoRoot, opts.PR) + if err != nil { + return Model{}, err + } + prCtx = &ctx + mode = reviewModePR + } + m := Model{ keys: defaultKeyMap(), focus: focusFiles, cwd: repoRoot, diffMode: gitint.DiffModeAll, + reviewMode: mode, statusSvc: gitint.NewStatusService(), diffSvc: gitint.NewDiffService(), + prSvc: prSvc, + prCtx: prCtx, + prDiffs: prDiffs, helpOpen: false, filePaneW: filePaneWidthDefault, treeCollapsed: make(map[string]bool), @@ -271,6 +312,9 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil } if msg.empty || len(msg.rows) == 0 { + if m.reviewMode == reviewModePR { + m.prDiffs[msg.path] = prDiffCacheEntry{empty: true} + } m.diffRows = nil m.diffCursor = 0 m.rowStarts = nil @@ -281,6 +325,9 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.newView.SetContent(noDiff) return m, nil } + if m.reviewMode == reviewModePR { + m.prDiffs[msg.path] = prDiffCacheEntry{rows: append([]diffview.DiffRow(nil), msg.rows...)} + } m.diffRows = msg.rows m.diffCursor = firstRenderableRow(m.diffRows) m.diffDirty = true @@ -389,10 +436,17 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } if key.Matches(msg, m.keys.Refresh) { diffview.ClearSyntaxCache() + if m.reviewMode == reviewModePR { + m.prDiffs = make(map[string]prDiffCacheEntry) + } m.loadingFiles = true return m, m.loadFilesCmd() } if key.Matches(msg, m.keys.ToggleMode) { + if m.reviewMode == reviewModePR { + m.setAlert("Diff mode toggle is unavailable in PR mode.") + return m, nil + } m.advanceDiffMode() if m.selectedF != "" { m.loadingDiff = true @@ -1383,6 +1437,13 @@ func (m Model) View() string { footerLines := []string{ lipgloss.NewStyle().Foreground(lipgloss.Color("244")).Render(footerHelpPlain), } + if m.reviewMode == reviewModePR && m.prCtx != nil { + prLine := truncateLinesToWidth( + fmt.Sprintf("Review target: PR #%d %s/%s", m.prCtx.Number, m.prCtx.Owner, m.prCtx.Repo), + m.width, + ) + footerLines = append(footerLines, lipgloss.NewStyle().Foreground(lipgloss.Color("111")).Render(prLine)) + } if staleCount := m.staleCommentCount(); staleCount > 0 { warn := truncateLinesToWidth( fmt.Sprintf("Warning: %d stale comment(s). They are marked with ⚠ and excluded from export.", staleCount), @@ -1451,11 +1512,15 @@ func (m Model) helpText() string { if m.leaderPending { leaderHint = "leader pending | " } + modeHint := "" + if m.reviewMode == reviewModePR && m.prCtx != nil { + modeHint = fmt.Sprintf("PR #%d %s/%s | ", m.prCtx.Number, m.prCtx.Owner, m.prCtx.Repo) + } if !m.helpOpen { - return leaderHint + "tab focus | m comments view | j/k move | ctrl-f/b page | ctrl-e/y scroll | enter open diff | z zoom/hide files | cmd | t mode | c/e/d comment | n/p comment nav | y export | C clear all | r refresh | ? help | q quit" + return leaderHint + modeHint + "tab focus | m comments view | j/k move | ctrl-f/b page | ctrl-e/y scroll | enter open diff | z zoom/hide files | cmd | t mode | c/e/d comment | n/p comment nav | y export | C clear all | r refresh | ? help | q quit" } return strings.Join([]string{ - "Global: q quit, tab switch focus, m comments view, t toggle diff mode, C clear all comments, ? toggle help", + modeHint + "Global: q quit, tab switch focus, m comments view, t toggle diff mode, C clear all comments, ? toggle help", "Leader: runs configured command from ~/.config/diffman/config.json", "Files pane: j/k move, ctrl-e/ctrl-y scroll, h/l tree nav, enter open diff, z toggle file pane width, r refresh", "Diff pane: j/k move cursor, ctrl-e/ctrl-y scroll, ctrl-f/ctrl-b page, g/G top/bottom, h focus files, z/l hide/show file list", @@ -1601,6 +1666,9 @@ func (m Model) renderFilesPane(width, height int) string { BorderForeground(borderColor) title := fmt.Sprintf("Files (%d)", len(m.fileItems)) + if m.reviewMode == reviewModePR && m.prCtx != nil { + title += fmt.Sprintf(" | PR #%d", m.prCtx.Number) + } if m.loadingFiles { title += " (loading...)" } @@ -2014,7 +2082,7 @@ func (m Model) renderDiffSidePane(width, height int, sideLabel, body string, wit if m.selectedF != "" { title = sideLabel + ": " + m.selectedF } - title += fmt.Sprintf(" [%s]", m.diffMode.String()) + title += fmt.Sprintf(" [%s]", m.diffModeLabel()) if m.loadingDiff { title += " (loading...)" } @@ -2025,6 +2093,16 @@ func (m Model) renderDiffSidePane(width, height int, sideLabel, body string, wit return paneStyle.Render(header + "\n\n" + body) } +func (m Model) diffModeLabel() string { + if m.reviewMode == reviewModePR { + if m.prCtx != nil { + return fmt.Sprintf("pr #%d", m.prCtx.Number) + } + return "pr" + } + return m.diffMode.String() +} + func (m *Model) resizePanes() { _, rightW := paneWidths(m.width, m.filePaneW, m.fileHidden, m.diffPaneMode() == diffPaneModeSplit) oldPaneW, newPaneW := m.diffSidePaneWidths(rightW) @@ -2042,6 +2120,15 @@ func (m *Model) resizePanes() { } func (m Model) loadFilesCmd() tea.Cmd { + if m.reviewMode == reviewModePR && m.prCtx != nil { + pr := *m.prCtx + service := m.prSvc + return func() tea.Msg { + items, err := service.ListFiles(context.Background(), pr) + return filesLoadedMsg{items: items, err: err} + } + } + cwd := m.cwd service := m.statusSvc return func() tea.Msg { @@ -2051,13 +2138,45 @@ func (m Model) loadFilesCmd() tea.Cmd { } func (m Model) loadCommentStaleCmd(items []gitint.FileItem, commentMap map[string]comments.Comment, mode gitint.DiffMode) tea.Cmd { - cwd := m.cwd - service := m.diffSvc itemSnapshot := append([]gitint.FileItem(nil), items...) commentSnapshot := make([]comments.Comment, 0, len(commentMap)) for _, c := range commentMap { commentSnapshot = append(commentSnapshot, c) } + + if m.reviewMode == reviewModePR && m.prCtx != nil { + pr := *m.prCtx + service := m.prSvc + cache := make(map[string]prDiffCacheEntry, len(m.prDiffs)) + for k, v := range m.prDiffs { + cache[k] = v + } + return func() tea.Msg { + stale, err := buildCommentStaleMapFromRowsLoader(itemSnapshot, commentSnapshot, func(path string) ([]diffview.DiffRow, bool, error) { + if cached, ok := cache[path]; ok { + return append([]diffview.DiffRow(nil), cached.rows...), cached.empty, nil + } + + d, err := service.Diff(context.Background(), pr, path) + if err != nil { + return nil, false, err + } + if strings.TrimSpace(d) == "" { + return nil, true, nil + } + + rows, err := diffview.ParseUnifiedDiff([]byte(d)) + if err != nil { + return nil, false, err + } + return rows, false, nil + }) + return commentStaleLoadedMsg{stale: stale, err: err} + } + } + + cwd := m.cwd + service := m.diffSvc return func() tea.Msg { stale, err := buildCommentStaleMap(context.Background(), cwd, service, itemSnapshot, commentSnapshot, mode) return commentStaleLoadedMsg{stale: stale, err: err} @@ -2065,6 +2184,32 @@ func (m Model) loadCommentStaleCmd(items []gitint.FileItem, commentMap map[strin } func (m Model) loadDiffCmd(path string) tea.Cmd { + if m.reviewMode == reviewModePR && m.prCtx != nil { + if cached, ok := m.prDiffs[path]; ok { + rows := append([]diffview.DiffRow(nil), cached.rows...) + return func() tea.Msg { + return diffLoadedMsg{path: path, rows: rows, empty: cached.empty} + } + } + pr := *m.prCtx + service := m.prSvc + return func() tea.Msg { + d, err := service.Diff(context.Background(), pr, path) + if err != nil { + return diffLoadedMsg{path: path, err: err} + } + if strings.TrimSpace(d) == "" { + return diffLoadedMsg{path: path, empty: true} + } + + rows, err := diffview.ParseUnifiedDiff([]byte(d)) + if err != nil { + return diffLoadedMsg{path: path, err: err} + } + return diffLoadedMsg{path: path, rows: rows} + } + } + cwd := m.cwd service := m.diffSvc mode := m.diffMode @@ -2769,6 +2914,16 @@ func buildCommentStaleMap( items []gitint.FileItem, allComments []comments.Comment, mode gitint.DiffMode, +) (map[string]bool, error) { + return buildCommentStaleMapFromLoader(items, allComments, func(path string) (string, error) { + return diffSvc.Diff(ctx, cwd, path, mode) + }) +} + +func buildCommentStaleMapFromRowsLoader( + items []gitint.FileItem, + allComments []comments.Comment, + loadRows func(path string) ([]diffview.DiffRow, bool, error), ) (map[string]bool, error) { stale := make(map[string]bool, len(allComments)) if len(allComments) == 0 { @@ -2792,7 +2947,7 @@ func buildCommentStaleMap( var firstErr error for path, group := range byPath { - d, err := diffSvc.Diff(ctx, cwd, path, mode) + rows, empty, err := loadRows(path) if err != nil { if firstErr == nil { firstErr = err @@ -2802,18 +2957,7 @@ func buildCommentStaleMap( } continue } - if strings.TrimSpace(d) == "" { - for _, c := range group { - stale[commentKey(c)] = true - } - continue - } - - rows, err := diffview.ParseUnifiedDiff([]byte(d)) - if err != nil { - if firstErr == nil { - firstErr = err - } + if empty || len(rows) == 0 { for _, c := range group { stale[commentKey(c)] = true } @@ -2843,3 +2987,24 @@ func buildCommentStaleMap( return stale, firstErr } + +func buildCommentStaleMapFromLoader( + items []gitint.FileItem, + allComments []comments.Comment, + loadDiff func(path string) (string, error), +) (map[string]bool, error) { + return buildCommentStaleMapFromRowsLoader(items, allComments, func(path string) ([]diffview.DiffRow, bool, error) { + d, err := loadDiff(path) + if err != nil { + return nil, false, err + } + if strings.TrimSpace(d) == "" { + return nil, true, nil + } + rows, err := diffview.ParseUnifiedDiff([]byte(d)) + if err != nil { + return nil, false, err + } + return rows, false, nil + }) +} diff --git a/internal/app/model_pr_cache_test.go b/internal/app/model_pr_cache_test.go new file mode 100644 index 0000000..3dc406e --- /dev/null +++ b/internal/app/model_pr_cache_test.go @@ -0,0 +1,105 @@ +package app + +import ( + "context" + "testing" + + "github.com/charmbracelet/bubbles/viewport" + + "diffman/internal/comments" + "diffman/internal/diffview" + "diffman/internal/git" + "diffman/internal/githubpr" +) + +type mockPRService struct { + diffCalls int + patches map[string]string +} + +func (m *mockPRService) ResolvePR(context.Context, string, string) (githubpr.Context, error) { + return githubpr.Context{}, nil +} + +func (m *mockPRService) ListFiles(context.Context, githubpr.Context) ([]git.FileItem, error) { + return nil, nil +} + +func (m *mockPRService) Diff(_ context.Context, _ githubpr.Context, path string) (string, error) { + m.diffCalls++ + return m.patches[path], nil +} + +func TestPRModeDiffCachingAvoidsSecondFetch(t *testing.T) { + service := &mockPRService{ + patches: map[string]string{ + "a.go": "diff --git a/a.go b/a.go\n--- a/a.go\n+++ b/a.go\n@@ -1 +1 @@\n-old\n+new\n", + }, + } + + m := Model{ + reviewMode: reviewModePR, + prSvc: service, + prCtx: &githubpr.Context{Owner: "acme", Repo: "widgets", Number: 42}, + prDiffs: make(map[string]prDiffCacheEntry), + oldView: viewport.New(80, 20), + newView: viewport.New(80, 20), + } + + first := m.loadDiffCmd("a.go")().(diffLoadedMsg) + if service.diffCalls != 1 { + t.Fatalf("expected first load to fetch from API once, got %d", service.diffCalls) + } + + nextModel, _ := m.Update(first) + m2 := nextModel.(Model) + + second := m2.loadDiffCmd("a.go")().(diffLoadedMsg) + if service.diffCalls != 1 { + t.Fatalf("expected second load to use cache, got %d API calls", service.diffCalls) + } + if second.err != nil { + t.Fatalf("cached load returned error: %v", second.err) + } + if second.empty || len(second.rows) == 0 { + t.Fatalf("cached load should return parsed rows") + } +} + +func TestPRModeStaleCheckUsesDiffCacheWhenAvailable(t *testing.T) { + service := &mockPRService{} + + m := Model{ + reviewMode: reviewModePR, + prSvc: service, + prCtx: &githubpr.Context{Owner: "acme", Repo: "widgets", Number: 42}, + prDiffs: map[string]prDiffCacheEntry{ + "a.go": { + rows: []diffview.DiffRow{ + {Kind: diffview.RowChange, Path: "a.go", OldLine: intPtr(1), NewLine: intPtr(1), OldText: "old", NewText: "new"}, + }, + }, + }, + } + + items := []git.FileItem{{Path: "a.go", Status: "M."}} + commentMap := map[string]comments.Comment{ + comments.AnchorKey("a.go", comments.SideNew, 1): { + Path: "a.go", + Side: comments.SideNew, + Line: 1, + Body: "test", + }, + } + + msg := m.loadCommentStaleCmd(items, commentMap, git.DiffModeAll)().(commentStaleLoadedMsg) + if msg.err != nil { + t.Fatalf("expected no error, got %v", msg.err) + } + if service.diffCalls != 0 { + t.Fatalf("expected stale check to use cache, got %d API calls", service.diffCalls) + } + if stale := msg.stale[comments.AnchorKey("a.go", comments.SideNew, 1)]; stale { + t.Fatalf("expected cached-line comment to be non-stale") + } +} diff --git a/internal/githubpr/gh.go b/internal/githubpr/gh.go new file mode 100644 index 0000000..6d2f6af --- /dev/null +++ b/internal/githubpr/gh.go @@ -0,0 +1,283 @@ +package githubpr + +import ( + "context" + "encoding/json" + "fmt" + "net/url" + "path" + "sort" + "strconv" + "strings" + + gitint "diffman/internal/git" + "diffman/internal/util" +) + +type ghService struct{} + +func (ghService) ResolvePR(ctx context.Context, cwd, input string) (Context, error) { + owner, repo, number, err := parsePRInput(input) + if err != nil { + return Context{}, err + } + if owner == "" || repo == "" { + owner, repo, err = discoverGitHubRepo(ctx, cwd) + if err != nil { + return Context{}, err + } + } + + var payload struct { + Title string `json:"title"` + HTMLURL string `json:"html_url"` + Head struct { + SHA string `json:"sha"` + Ref string `json:"ref"` + } `json:"head"` + Base struct { + Ref string `json:"ref"` + } `json:"base"` + } + + body, err := util.Run(ctx, "", "gh", "api", fmt.Sprintf("repos/%s/%s/pulls/%d", owner, repo, number)) + if err != nil { + return Context{}, err + } + if err := json.Unmarshal([]byte(body), &payload); err != nil { + return Context{}, fmt.Errorf("parse pr metadata: %w", err) + } + + return Context{ + Owner: owner, + Repo: repo, + Number: number, + Title: payload.Title, + URL: payload.HTMLURL, + HeadSHA: payload.Head.SHA, + HeadRef: payload.Head.Ref, + BaseRef: payload.Base.Ref, + }, nil +} + +func (ghService) ListFiles(ctx context.Context, pr Context) ([]gitint.FileItem, error) { + files, err := listPRFiles(ctx, pr) + if err != nil { + return nil, err + } + + out := make([]gitint.FileItem, 0, len(files)) + for _, file := range files { + out = append(out, gitint.FileItem{ + Path: file.Filename, + Status: mapGitHubStatus(file.Status), + HasStaged: false, + HasUnstaged: true, + }) + } + + sort.Slice(out, func(i, j int) bool { + return out[i].Path < out[j].Path + }) + + return out, nil +} + +func (ghService) Diff(ctx context.Context, pr Context, targetPath string) (string, error) { + files, err := listPRFiles(ctx, pr) + if err != nil { + return "", err + } + + for _, file := range files { + if file.Filename != targetPath { + continue + } + return buildUnifiedPatch(file), nil + } + + return "", nil +} + +type prFile struct { + Filename string `json:"filename"` + PreviousFilename string `json:"previous_filename"` + Status string `json:"status"` + Patch string `json:"patch"` +} + +func listPRFiles(ctx context.Context, pr Context) ([]prFile, error) { + body, err := util.Run( + ctx, + "", + "gh", + "api", + "--paginate", + "--slurp", + fmt.Sprintf("repos/%s/%s/pulls/%d/files?per_page=100", pr.Owner, pr.Repo, pr.Number), + ) + if err != nil { + return nil, err + } + + files, err := parsePaginatedFilesJSON([]byte(body)) + if err != nil { + return nil, err + } + return files, nil +} + +func parsePaginatedFilesJSON(body []byte) ([]prFile, error) { + var files []prFile + if err := json.Unmarshal(body, &files); err == nil { + return files, nil + } + + var pages [][]prFile + if err := json.Unmarshal(body, &pages); err != nil { + return nil, fmt.Errorf("parse pr files: %w", err) + } + + total := 0 + for _, page := range pages { + total += len(page) + } + files = make([]prFile, 0, total) + for _, page := range pages { + files = append(files, page...) + } + return files, nil +} + +func parsePRInput(input string) (owner, repo string, number int, err error) { + raw := strings.TrimSpace(input) + if raw == "" { + return "", "", 0, fmt.Errorf("pull request input cannot be empty") + } + + if n, convErr := strconv.Atoi(raw); convErr == nil { + if n <= 0 { + return "", "", 0, fmt.Errorf("pull request number must be positive") + } + return "", "", n, nil + } + + u, parseErr := url.Parse(raw) + if parseErr != nil { + return "", "", 0, fmt.Errorf("invalid pull request input %q", input) + } + if !strings.EqualFold(u.Host, "github.com") { + return "", "", 0, fmt.Errorf("unsupported pull request host %q", u.Host) + } + + parts := strings.Split(strings.Trim(path.Clean(u.Path), "/"), "/") + if len(parts) < 4 || parts[2] != "pull" { + return "", "", 0, fmt.Errorf("invalid pull request url %q", input) + } + + n, convErr := strconv.Atoi(parts[3]) + if convErr != nil || n <= 0 { + return "", "", 0, fmt.Errorf("invalid pull request number in url %q", input) + } + + return parts[0], parts[1], n, nil +} + +func discoverGitHubRepo(ctx context.Context, cwd string) (owner, repo string, err error) { + out, err := util.Run(ctx, cwd, "git", "config", "--get", "remote.origin.url") + if err != nil { + return "", "", err + } + + raw := strings.TrimSpace(out) + if raw == "" { + return "", "", fmt.Errorf("git remote.origin.url is empty") + } + + if strings.HasPrefix(raw, "git@github.com:") { + repoPath := strings.TrimPrefix(raw, "git@github.com:") + return splitOwnerRepo(repoPath) + } + + if strings.HasPrefix(raw, "https://github.com/") || strings.HasPrefix(raw, "http://github.com/") || strings.HasPrefix(raw, "ssh://git@github.com/") { + u, parseErr := url.Parse(raw) + if parseErr != nil { + return "", "", fmt.Errorf("parse git remote url: %w", parseErr) + } + if !strings.EqualFold(u.Host, "github.com") { + return "", "", fmt.Errorf("unsupported git host %q", u.Host) + } + return splitOwnerRepo(strings.TrimPrefix(u.Path, "/")) + } + + return "", "", fmt.Errorf("unsupported git remote url %q", raw) +} + +func splitOwnerRepo(repoPath string) (owner, repo string, err error) { + clean := strings.TrimSuffix(strings.TrimSpace(repoPath), ".git") + parts := strings.Split(clean, "/") + if len(parts) < 2 { + return "", "", fmt.Errorf("invalid github repo path %q", repoPath) + } + owner = parts[len(parts)-2] + repo = parts[len(parts)-1] + if owner == "" || repo == "" { + return "", "", fmt.Errorf("invalid github repo path %q", repoPath) + } + return owner, repo, nil +} + +func mapGitHubStatus(status string) string { + switch strings.ToLower(strings.TrimSpace(status)) { + case "added": + return "A." + case "removed": + return "D." + case "renamed": + return "R." + case "copied": + return "C." + case "modified", "changed": + return "M." + default: + return ".." + } +} + +func buildUnifiedPatch(file prFile) string { + patch := strings.TrimSpace(file.Patch) + if patch == "" { + return "" + } + if strings.HasPrefix(patch, "diff --git ") { + return patch + } + + oldPath := file.Filename + newPath := file.Filename + switch strings.ToLower(strings.TrimSpace(file.Status)) { + case "added": + oldPath = "/dev/null" + newPath = "b/" + file.Filename + case "removed": + oldPath = "a/" + file.Filename + newPath = "/dev/null" + case "renamed": + oldName := strings.TrimSpace(file.PreviousFilename) + if oldName == "" { + oldName = file.Filename + } + oldPath = "a/" + oldName + newPath = "b/" + file.Filename + default: + oldPath = "a/" + file.Filename + newPath = "b/" + file.Filename + } + + header := []string{ + fmt.Sprintf("diff --git a/%s b/%s", file.Filename, file.Filename), + "--- " + oldPath, + "+++ " + newPath, + } + return strings.Join(header, "\n") + "\n" + patch + "\n" +} diff --git a/internal/githubpr/gh_test.go b/internal/githubpr/gh_test.go new file mode 100644 index 0000000..39cddac --- /dev/null +++ b/internal/githubpr/gh_test.go @@ -0,0 +1,86 @@ +package githubpr + +import ( + "strings" + "testing" +) + +func TestParsePRInput_Number(t *testing.T) { + owner, repo, number, err := parsePRInput("123") + if err != nil { + t.Fatalf("parsePRInput returned error: %v", err) + } + if owner != "" || repo != "" { + t.Fatalf("expected empty owner/repo for numeric input, got %q/%q", owner, repo) + } + if number != 123 { + t.Fatalf("expected number=123, got %d", number) + } +} + +func TestParsePRInput_URL(t *testing.T) { + owner, repo, number, err := parsePRInput("https://github.com/acme/widgets/pull/42") + if err != nil { + t.Fatalf("parsePRInput returned error: %v", err) + } + if owner != "acme" || repo != "widgets" || number != 42 { + t.Fatalf("unexpected parse result: owner=%q repo=%q number=%d", owner, repo, number) + } +} + +func TestSplitOwnerRepo(t *testing.T) { + owner, repo, err := splitOwnerRepo("acme/widgets.git") + if err != nil { + t.Fatalf("splitOwnerRepo returned error: %v", err) + } + if owner != "acme" || repo != "widgets" { + t.Fatalf("unexpected split result: owner=%q repo=%q", owner, repo) + } +} + +func TestMapGitHubStatus(t *testing.T) { + tests := map[string]string{ + "added": "A.", + "removed": "D.", + "modified": "M.", + "renamed": "R.", + "copied": "C.", + "other": "..", + } + + for in, want := range tests { + if got := mapGitHubStatus(in); got != want { + t.Fatalf("mapGitHubStatus(%q) = %q, want %q", in, got, want) + } + } +} + +func TestBuildUnifiedPatch_WrapsPatchBody(t *testing.T) { + patch := buildUnifiedPatch(prFile{ + Filename: "main.go", + Status: "modified", + Patch: "@@ -1 +1 @@\n-old\n+new", + }) + + if patch == "" { + t.Fatal("expected non-empty patch") + } + if want := "diff --git a/main.go b/main.go"; !strings.HasPrefix(patch, want) { + t.Fatalf("expected patch to start with %q, got %q", want, patch) + } +} + +func TestParsePaginatedFilesJSON_SlurpFormat(t *testing.T) { + body := []byte(`[[{"filename":"a.go","status":"modified","patch":"@@ -1 +1 @@\n-a\n+b"}],[{"filename":"b.go","status":"added","patch":"@@ -0,0 +1 @@\n+b"}]]`) + + files, err := parsePaginatedFilesJSON(body) + if err != nil { + t.Fatalf("parsePaginatedFilesJSON returned error: %v", err) + } + if len(files) != 2 { + t.Fatalf("expected 2 files, got %d", len(files)) + } + if files[0].Filename != "a.go" || files[1].Filename != "b.go" { + t.Fatalf("unexpected filenames: %#v", files) + } +} diff --git a/internal/githubpr/service.go b/internal/githubpr/service.go new file mode 100644 index 0000000..b84003d --- /dev/null +++ b/internal/githubpr/service.go @@ -0,0 +1,28 @@ +package githubpr + +import ( + "context" + + gitint "diffman/internal/git" +) + +type Context struct { + Owner string + Repo string + Number int + Title string + URL string + HeadSHA string + HeadRef string + BaseRef string +} + +type Service interface { + ResolvePR(ctx context.Context, cwd, input string) (Context, error) + ListFiles(ctx context.Context, pr Context) ([]gitint.FileItem, error) + Diff(ctx context.Context, pr Context, path string) (string, error) +} + +func NewService() Service { + return ghService{} +} From 07a355363ed8d0fd504311fcb7c551da8cccd94c Mon Sep 17 00:00:00 2001 From: Michael Rose Date: Tue, 24 Mar 2026 08:12:36 +0100 Subject: [PATCH 3/4] add submit review functionality --- internal/app/keymap.go | 2 + internal/app/model.go | 252 +++++++++++++++++++++++++++- internal/app/model_pr_cache_test.go | 44 ++++- internal/githubpr/gh.go | 73 ++++++++ internal/githubpr/gh_test.go | 28 ++++ internal/githubpr/service.go | 2 + 6 files changed, 397 insertions(+), 4 deletions(-) diff --git a/internal/app/keymap.go b/internal/app/keymap.go index 8ca3b1e..dcd3d16 100644 --- a/internal/app/keymap.go +++ b/internal/app/keymap.go @@ -25,6 +25,7 @@ type KeyMap struct { NextComment key.Binding PrevComment key.Binding Export key.Binding + SubmitReview key.Binding ClearAll key.Binding CommentsView key.Binding } @@ -52,6 +53,7 @@ func defaultKeyMap() KeyMap { NextComment: key.NewBinding(key.WithKeys("n"), key.WithHelp("n", "next comment")), PrevComment: key.NewBinding(key.WithKeys("p"), key.WithHelp("p", "prev comment")), Export: key.NewBinding(key.WithKeys("y"), key.WithHelp("y", "copy export")), + SubmitReview: key.NewBinding(key.WithKeys("s"), key.WithHelp("s", "submit PR comments")), ClearAll: key.NewBinding(key.WithKeys("C"), key.WithHelp("C", "clear all comments")), CommentsView: key.NewBinding(key.WithKeys("m"), key.WithHelp("m", "comments view")), } diff --git a/internal/app/model.go b/internal/app/model.go index cd36bb2..c2b5456 100644 --- a/internal/app/model.go +++ b/internal/app/model.go @@ -89,6 +89,19 @@ type leaderCommandResultMsg struct { err error } +type submitReviewResultMsg struct { + submitted []comments.Comment + err error +} + +type reviewEvent string + +const ( + reviewEventComment reviewEvent = "COMMENT" + reviewEventApprove reviewEvent = "APPROVE" + reviewEventRequestChanges reviewEvent = "REQUEST_CHANGES" +) + type commentAnchor struct { Path string Side comments.Side @@ -147,6 +160,13 @@ type Model struct { commentEditAnchor *commentAnchor commentEditKey string + reviewInputActive bool + reviewInputModel textinput.Model + reviewInputErr string + reviewActionModal bool + reviewBodyDraft string + reviewDraft []comments.Comment + alertMsg string alertUntil time.Time clearConfirmModal bool @@ -195,6 +215,13 @@ func NewModelWithOptions(opts Options) (Model, error) { commentInput.Cursor.Style = lipgloss.NewStyle().Foreground(lipgloss.Color("51")) commentInput.PlaceholderStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("244")) + reviewInput := textinput.New() + reviewInput.Prompt = "" + reviewInput.Placeholder = "Type review summary" + reviewInput.CharLimit = 4096 + reviewInput.Cursor.Style = lipgloss.NewStyle().Foreground(lipgloss.Color("51")) + reviewInput.PlaceholderStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("244")) + prSvc := githubpr.NewService() var prCtx *githubpr.Context prDiffs := make(map[string]prDiffCacheEntry) @@ -228,6 +255,7 @@ func NewModelWithOptions(opts Options) (Model, error) { comments: commentMap, leaderCommands: appConfig.LeaderCommands, commentInputModel: commentInput, + reviewInputModel: reviewInput, diffDirty: true, oldWidth: -1, newWidth: -1, @@ -368,10 +396,33 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.loadingFiles = true return m, m.loadFilesCmd() + case submitReviewResultMsg: + m.resetReviewSubmissionState() + if msg.err != nil { + m.setAlert(fmt.Sprintf("submit review failed: %v", msg.err)) + return m, nil + } + if len(msg.submitted) == 0 { + m.setAlert("No comments submitted.") + return m, nil + } + if err := m.removeSubmittedComments(msg.submitted); err != nil { + m.setAlert(fmt.Sprintf("submitted to GitHub, but failed to update local drafts: %v", err)) + return m, nil + } + m.setAlert(fmt.Sprintf("Submitted %d comment(s) to GitHub.", len(msg.submitted))) + return m, nil + case tea.KeyMsg: if m.commentInputActive { return m.handleCommentInput(msg) } + if m.reviewInputActive { + return m.handleReviewInput(msg) + } + if m.reviewActionModal { + return m.handleReviewAction(msg) + } if m.clearConfirmModal { return m.handleClearConfirm(msg) } @@ -465,6 +516,9 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.clearConfirmModal = true return m, nil } + if key.Matches(msg, m.keys.SubmitReview) { + return m.handleSubmitPRComments() + } if m.focus == focusFiles { return m.updateFilesPane(msg) @@ -954,6 +1008,8 @@ func (m *Model) commentsPageSize() int { dockHeight := 0 if m.commentInputActive { dockHeight = lipgloss.Height(m.renderCommentDock()) + } else if m.reviewInputActive { + dockHeight = lipgloss.Height(m.renderReviewDock()) } else if m.alertMsg != "" { dockHeight = lipgloss.Height(m.renderAlertDock()) } @@ -1181,6 +1237,80 @@ func (m Model) handleExportComments() (tea.Model, tea.Cmd) { return m, m.exportCommentsCmd() } +func (m Model) handleSubmitPRComments() (tea.Model, tea.Cmd) { + if m.reviewMode != reviewModePR || m.prCtx == nil { + m.setAlert("PR submission is only available in PR mode.") + return m, nil + } + + draft := m.exportableComments() + if len(draft) == 0 { + m.setAlert("No non-stale comments to submit.") + return m, nil + } + + m.reviewDraft = append([]comments.Comment(nil), draft...) + m.reviewBodyDraft = "" + m.reviewInputErr = "" + m.reviewInputActive = true + m.reviewActionModal = false + m.reviewInputModel.SetValue("") + cmd := m.reviewInputModel.Focus() + m.reviewInputModel.CursorEnd() + return m, cmd +} + +func (m Model) handleReviewInput(msg tea.KeyMsg) (tea.Model, tea.Cmd) { + switch msg.Type { + case tea.KeyEsc: + m.resetReviewSubmissionState() + return m, nil + case tea.KeyEnter: + m.reviewBodyDraft = strings.TrimSpace(m.reviewInputModel.Value()) + m.reviewInputActive = false + m.reviewActionModal = true + m.reviewInputModel.Blur() + return m, nil + } + + var cmd tea.Cmd + m.reviewInputModel, cmd = m.reviewInputModel.Update(msg) + m.reviewInputErr = "" + return m, cmd +} + +func (m Model) handleReviewAction(msg tea.KeyMsg) (tea.Model, tea.Cmd) { + if msg.Type == tea.KeyEsc { + m.resetReviewSubmissionState() + return m, nil + } + + if msg.Type == tea.KeyRunes { + switch msg.String() { + case "a", "A": + m.reviewActionModal = false + return m, m.submitReviewCmd(m.reviewDraft, m.reviewBodyDraft, reviewEventApprove) + case "c", "C": + m.reviewActionModal = false + return m, m.submitReviewCmd(m.reviewDraft, m.reviewBodyDraft, reviewEventComment) + case "r", "R": + m.reviewActionModal = false + return m, m.submitReviewCmd(m.reviewDraft, m.reviewBodyDraft, reviewEventRequestChanges) + } + } + return m, nil +} + +func (m *Model) resetReviewSubmissionState() { + m.reviewInputActive = false + m.reviewActionModal = false + m.reviewInputErr = "" + m.reviewBodyDraft = "" + m.reviewDraft = nil + m.reviewInputModel.SetValue("") + m.reviewInputModel.Blur() +} + func (m Model) handleCommentInput(msg tea.KeyMsg) (tea.Model, tea.Cmd) { switch msg.Type { case tea.KeyEsc: @@ -1394,6 +1524,47 @@ func (m *Model) clearAllComments() { m.refreshDiffContent() } +func (m *Model) removeSubmittedComments(submitted []comments.Comment) error { + if len(submitted) == 0 { + return nil + } + + remove := make(map[string]bool, len(submitted)) + for _, c := range submitted { + remove[commentKey(c)] = true + } + + prevComments := m.comments + prevStale := m.commentStale + + nextComments := make(map[string]comments.Comment, len(m.comments)) + for k, c := range m.comments { + if remove[k] { + continue + } + nextComments[k] = c + } + nextStale := make(map[string]bool, len(m.commentStale)) + for k, v := range m.commentStale { + if remove[k] { + continue + } + nextStale[k] = v + } + + m.comments = nextComments + m.commentStale = nextStale + if err := m.persistComments(); err != nil { + m.comments = prevComments + m.commentStale = prevStale + return err + } + + m.diffDirty = true + m.refreshDiffContent() + return nil +} + func (m *Model) jumpToComment(direction int) { rows := m.commentRowIndices() if len(rows) == 0 { @@ -1459,6 +1630,9 @@ func (m Model) View() string { if m.commentInputActive { dock = m.renderCommentDock() dockHeight = lipgloss.Height(dock) + } else if m.reviewInputActive { + dock = m.renderReviewDock() + dockHeight = lipgloss.Height(dock) } else if m.alertMsg != "" { dock = m.renderAlertDock() dockHeight = lipgloss.Height(dock) @@ -1504,6 +1678,9 @@ func (m Model) View() string { if m.clearConfirmModal { body = overlayCentered(body, m.renderClearAllConfirmModal(), m.width, lipgloss.Height(body)) } + if m.reviewActionModal { + body = overlayCentered(body, m.renderReviewActionModal(), m.width, lipgloss.Height(body)) + } return lipgloss.JoinVertical(lipgloss.Left, body, footer) } @@ -1517,7 +1694,7 @@ func (m Model) helpText() string { modeHint = fmt.Sprintf("PR #%d %s/%s | ", m.prCtx.Number, m.prCtx.Owner, m.prCtx.Repo) } if !m.helpOpen { - return leaderHint + modeHint + "tab focus | m comments view | j/k move | ctrl-f/b page | ctrl-e/y scroll | enter open diff | z zoom/hide files | cmd | t mode | c/e/d comment | n/p comment nav | y export | C clear all | r refresh | ? help | q quit" + return leaderHint + modeHint + "tab focus | m comments view | j/k move | ctrl-f/b page | ctrl-e/y scroll | enter open diff | z zoom/hide files | cmd | t mode | c/e/d comment | n/p comment nav | y export | s submit PR | C clear all | r refresh | ? help | q quit" } return strings.Join([]string{ modeHint + "Global: q quit, tab switch focus, m comments view, t toggle diff mode, C clear all comments, ? toggle help", @@ -1525,7 +1702,7 @@ func (m Model) helpText() string { "Files pane: j/k move, ctrl-e/ctrl-y scroll, h/l tree nav, enter open diff, z toggle file pane width, r refresh", "Diff pane: j/k move cursor, ctrl-e/ctrl-y scroll, ctrl-f/ctrl-b page, g/G top/bottom, h focus files, z/l hide/show file list", "Comments view: j/k move, ctrl-e/ctrl-y scroll, ctrl-f/ctrl-b page, g/G top/bottom, e edit, d delete, enter jump to diff", - "Comments: c create, e edit, d delete, n/p next/prev, y export to clipboard", + "Comments: c create, e edit, d delete, n/p next/prev, y export to clipboard, s submit PR comments", }, "\n") } @@ -1586,6 +1763,26 @@ func (m Model) renderCommentDock() string { return m.renderDockPanel(title, lipgloss.Color("39"), lipgloss.Color("39"), body) } +func (m Model) renderReviewDock() string { + contentW := max(10, m.width-2) + bodyInnerW := max(1, contentW-4) + inputWidth := max(1, bodyInnerW-4) + input := m.reviewInputModel + input.Width = inputWidth + inputBox := lipgloss.NewStyle(). + Width(bodyInnerW). + MaxWidth(bodyInnerW). + Border(lipgloss.NormalBorder()). + BorderForeground(lipgloss.Color("111")). + Padding(0, 1). + Render(input.View()) + hint := lipgloss.NewStyle().Foreground(lipgloss.Color("244")).Render( + ansi.Truncate("Enter continue | Esc cancel", bodyInnerW, ""), + ) + body := strings.Join([]string{inputBox, "", hint}, "\n") + return m.renderDockPanel("Review Body", lipgloss.Color("111"), lipgloss.Color("111"), body) +} + func (m Model) renderAlertDock() string { hint := lipgloss.NewStyle().Foreground(lipgloss.Color("244")).Render("Auto-hides after 3s") body := strings.Join([]string{ @@ -1628,6 +1825,42 @@ func (m Model) renderClearAllConfirmModal() string { Render(title + "\n" + bodyBlock) } +func (m Model) renderReviewActionModal() string { + body := strings.Join([]string{ + "Choose review action:", + "", + lipgloss.NewStyle().Foreground(lipgloss.Color("78")).Render("A approve"), + lipgloss.NewStyle().Foreground(lipgloss.Color("252")).Render("C comment"), + lipgloss.NewStyle().Foreground(lipgloss.Color("203")).Render("R request changes"), + "", + lipgloss.NewStyle().Foreground(lipgloss.Color("244")).Render("Esc cancel"), + }, "\n") + + width := 54 + if m.width > 0 && m.width-6 < width { + width = max(24, m.width-6) + } + + title := lipgloss.NewStyle(). + Width(max(1, width-2)). + Padding(0, 1). + Bold(true). + Foreground(lipgloss.Color("230")). + Background(lipgloss.Color("111")). + Render("Submit Review") + + bodyBlock := lipgloss.NewStyle(). + Width(max(1, width-2)). + Padding(1, 2). + Render(body) + + return lipgloss.NewStyle(). + Width(width). + Border(lipgloss.RoundedBorder()). + BorderForeground(lipgloss.Color("111")). + Render(title + "\n" + bodyBlock) +} + func (m Model) renderDockPanel(title string, titleColor, borderColor lipgloss.Color, body string) string { contentW := max(10, m.width-2) titleText := ansi.Truncate(title, max(1, contentW-2), "") @@ -2239,6 +2472,21 @@ func (m Model) exportCommentsCmd() tea.Cmd { } } +func (m Model) submitReviewCmd(draft []comments.Comment, body string, event reviewEvent) tea.Cmd { + pr := m.prCtx + service := m.prSvc + snapshot := append([]comments.Comment(nil), draft...) + bodySnapshot := strings.TrimSpace(body) + eventSnapshot := string(event) + return func() tea.Msg { + if pr == nil { + return submitReviewResultMsg{err: fmt.Errorf("missing PR context")} + } + err := service.SubmitReviewComments(context.Background(), *pr, bodySnapshot, eventSnapshot, snapshot) + return submitReviewResultMsg{submitted: snapshot, err: err} + } +} + func (m Model) execLeaderCommandCmd(key, command string) tea.Cmd { sh := strings.TrimSpace(os.Getenv("SHELL")) if sh == "" { diff --git a/internal/app/model_pr_cache_test.go b/internal/app/model_pr_cache_test.go index 3dc406e..89977b6 100644 --- a/internal/app/model_pr_cache_test.go +++ b/internal/app/model_pr_cache_test.go @@ -13,8 +13,9 @@ import ( ) type mockPRService struct { - diffCalls int - patches map[string]string + diffCalls int + submitCalls int + patches map[string]string } func (m *mockPRService) ResolvePR(context.Context, string, string) (githubpr.Context, error) { @@ -30,6 +31,11 @@ func (m *mockPRService) Diff(_ context.Context, _ githubpr.Context, path string) return m.patches[path], nil } +func (m *mockPRService) SubmitReviewComments(context.Context, githubpr.Context, string, string, []comments.Comment) error { + m.submitCalls++ + return nil +} + func TestPRModeDiffCachingAvoidsSecondFetch(t *testing.T) { service := &mockPRService{ patches: map[string]string{ @@ -103,3 +109,37 @@ func TestPRModeStaleCheckUsesDiffCacheWhenAvailable(t *testing.T) { t.Fatalf("expected cached-line comment to be non-stale") } } + +func TestSubmitPRCommentsRemovesSubmittedDrafts(t *testing.T) { + service := &mockPRService{} + key := comments.AnchorKey("a.go", comments.SideNew, 1) + draft := comments.Comment{Path: "a.go", Side: comments.SideNew, Line: 1, Body: "ship it"} + + m := Model{ + reviewMode: reviewModePR, + prSvc: service, + prCtx: &githubpr.Context{Owner: "acme", Repo: "widgets", Number: 42}, + commentStore: comments.NewStore(t.TempDir()), + comments: map[string]comments.Comment{ + key: draft, + }, + commentStale: map[string]bool{key: false}, + oldView: viewport.New(80, 20), + newView: viewport.New(80, 20), + } + + cmd := m.submitReviewCmd([]comments.Comment{draft}, "body", reviewEventComment) + msg := cmd().(submitReviewResultMsg) + nextModel, _ := m.Update(msg) + m2 := nextModel.(Model) + + if service.submitCalls != 1 { + t.Fatalf("expected one submit call, got %d", service.submitCalls) + } + if len(m2.comments) != 0 { + t.Fatalf("expected submitted comment to be removed locally") + } + if len(m2.commentStale) != 0 { + t.Fatalf("expected stale map entry removed with submitted comment") + } +} diff --git a/internal/githubpr/gh.go b/internal/githubpr/gh.go index 6d2f6af..11bce57 100644 --- a/internal/githubpr/gh.go +++ b/internal/githubpr/gh.go @@ -10,6 +10,7 @@ import ( "strconv" "strings" + "diffman/internal/comments" gitint "diffman/internal/git" "diffman/internal/util" ) @@ -99,6 +100,78 @@ func (ghService) Diff(ctx context.Context, pr Context, targetPath string) (strin return "", nil } +func (ghService) SubmitReviewComments(ctx context.Context, pr Context, body, event string, draft []comments.Comment) error { + if len(draft) == 0 { + return nil + } + payload := buildSubmitReviewPayload(pr, body, event, draft) + payloadJSON, err := json.Marshal(payload) + if err != nil { + return fmt.Errorf("marshal review payload: %w", err) + } + + _, err = util.RunWithStdin( + ctx, + "", + string(payloadJSON), + "gh", + "api", + "--method", + "POST", + fmt.Sprintf("repos/%s/%s/pulls/%d/reviews", pr.Owner, pr.Repo, pr.Number), + "--input", + "-", + ) + if err != nil { + return err + } + return nil +} + +type reviewCommentPayload struct { + Path string `json:"path"` + Body string `json:"body"` + Line int `json:"line"` + Side string `json:"side"` +} + +type submitReviewPayload struct { + Body string `json:"body,omitempty"` + Event string `json:"event"` + CommitID string `json:"commit_id,omitempty"` + Comments []reviewCommentPayload `json:"comments"` +} + +func buildSubmitReviewPayload(pr Context, body, event string, draft []comments.Comment) submitReviewPayload { + reviewBody := strings.TrimSpace(body) + if reviewBody == "" { + reviewBody = "Review comments submitted via diffman." + } + reviewEvent := strings.ToUpper(strings.TrimSpace(event)) + if reviewEvent == "" { + reviewEvent = "COMMENT" + } + payload := submitReviewPayload{ + Body: reviewBody, + Event: reviewEvent, + CommitID: pr.HeadSHA, + Comments: make([]reviewCommentPayload, 0, len(draft)), + } + for _, c := range draft { + side := "RIGHT" + if c.Side == comments.SideOld { + side = "LEFT" + } + payload.Comments = append(payload.Comments, reviewCommentPayload{ + Path: c.Path, + Body: c.Body, + Line: c.Line, + Side: side, + }) + } + return payload +} + type prFile struct { Filename string `json:"filename"` PreviousFilename string `json:"previous_filename"` diff --git a/internal/githubpr/gh_test.go b/internal/githubpr/gh_test.go index 39cddac..50f093a 100644 --- a/internal/githubpr/gh_test.go +++ b/internal/githubpr/gh_test.go @@ -3,6 +3,8 @@ package githubpr import ( "strings" "testing" + + "diffman/internal/comments" ) func TestParsePRInput_Number(t *testing.T) { @@ -84,3 +86,29 @@ func TestParsePaginatedFilesJSON_SlurpFormat(t *testing.T) { t.Fatalf("unexpected filenames: %#v", files) } } + +func TestBuildSubmitReviewPayload_MapsCommentSides(t *testing.T) { + payload := buildSubmitReviewPayload(Context{HeadSHA: "abc123"}, "summary", "APPROVE", []comments.Comment{ + {Path: "a.go", Side: comments.SideNew, Line: 7, Body: "new-side"}, + {Path: "a.go", Side: comments.SideOld, Line: 3, Body: "old-side"}, + }) + + if payload.Event != "APPROVE" { + t.Fatalf("expected APPROVE event, got %q", payload.Event) + } + if payload.Body != "summary" { + t.Fatalf("expected custom review body") + } + if payload.CommitID != "abc123" { + t.Fatalf("expected commit id to match head sha") + } + if len(payload.Comments) != 2 { + t.Fatalf("expected 2 payload comments, got %d", len(payload.Comments)) + } + if payload.Comments[0].Side != "RIGHT" { + t.Fatalf("expected new-side comment mapped to RIGHT, got %q", payload.Comments[0].Side) + } + if payload.Comments[1].Side != "LEFT" { + t.Fatalf("expected old-side comment mapped to LEFT, got %q", payload.Comments[1].Side) + } +} diff --git a/internal/githubpr/service.go b/internal/githubpr/service.go index b84003d..036bc7b 100644 --- a/internal/githubpr/service.go +++ b/internal/githubpr/service.go @@ -3,6 +3,7 @@ package githubpr import ( "context" + "diffman/internal/comments" gitint "diffman/internal/git" ) @@ -21,6 +22,7 @@ type Service interface { ResolvePR(ctx context.Context, cwd, input string) (Context, error) ListFiles(ctx context.Context, pr Context) ([]gitint.FileItem, error) Diff(ctx context.Context, pr Context, path string) (string, error) + SubmitReviewComments(ctx context.Context, pr Context, body, event string, draft []comments.Comment) error } func NewService() Service { From d08e3382ccf2545e67452f028c277bce4d1772b2 Mon Sep 17 00:00:00 2001 From: Michael Rose Date: Tue, 24 Mar 2026 09:10:01 +0100 Subject: [PATCH 4/4] finish implementation --- README.md | 11 + cmd/diffman/main.go | 11 +- internal/app/model.go | 358 ++++++++++++++++++++++++++- internal/app/model_pr_cache_test.go | 4 + internal/app/model_pr_picker_test.go | 85 +++++++ internal/githubpr/gh.go | 76 ++++++ internal/githubpr/service.go | 12 + 7 files changed, 552 insertions(+), 5 deletions(-) create mode 100644 internal/app/model_pr_picker_test.go diff --git a/README.md b/README.md index fcb7378..15a4459 100644 --- a/README.md +++ b/README.md @@ -14,6 +14,7 @@ https://github.com/user-attachments/assets/3cd5cae6-3137-4b85-bad8-30e8eddcc5bd - Inline comment display in the diff panes. - Comment list view across all files. - Export comments to clipboard in a review-friendly format. +- GitHub PR mode with open-PR picker and review submission. - Leader key commands (``) configurable via user config. ## Requirements @@ -49,6 +50,14 @@ diffman `diffman` discovers the repository root automatically and shows changed files. +GitHub PR mode: + +```bash +diffman -pr # pick from open PRs in current repo +diffman -pr -pr-ref 123 # open PR number directly +diffman -pr -pr-ref https://github.com/org/repo/pull/123 +``` + ## UI Overview The app has three views: @@ -71,6 +80,7 @@ Focus moves with `tab`. - ``: run configured leader command - `?`: toggle expanded help - `q`: quit (except in comments view, where it closes comments view) +- In PR mode: `q` from an active PR returns to PR picker; `q` again quits. ### Files View @@ -99,6 +109,7 @@ Directory navigation behavior: - `d`: delete comment on current line - `n` / `p`: jump next/previous comment in current diff - `y`: copy exported comments to clipboard +- `s`: submit PR review (enter body, then choose approve/comment/request changes) - `z` or `l`: hide/show file pane - `h`: focus files view diff --git a/cmd/diffman/main.go b/cmd/diffman/main.go index dfbb116..bbe0b4c 100644 --- a/cmd/diffman/main.go +++ b/cmd/diffman/main.go @@ -11,11 +11,16 @@ import ( ) func main() { - var pr string - flag.StringVar(&pr, "pr", "", "GitHub pull request number or URL") + var prMode bool + var prRef string + flag.BoolVar(&prMode, "pr", false, "Launch in GitHub PR mode (open PR picker)") + flag.StringVar(&prRef, "pr-ref", "", "GitHub pull request number or URL") flag.Parse() + if prRef != "" { + prMode = true + } - model, err := app.NewModelWithOptions(app.Options{PR: pr}) + model, err := app.NewModelWithOptions(app.Options{PR: prRef, PRPicker: prMode && prRef == ""}) if err != nil { fmt.Fprintf(os.Stderr, "failed to initialize app: %v\n", err) os.Exit(1) diff --git a/internal/app/model.go b/internal/app/model.go index c2b5456..73b363e 100644 --- a/internal/app/model.go +++ b/internal/app/model.go @@ -6,6 +6,7 @@ import ( "os" "os/exec" "sort" + "strconv" "strings" "time" @@ -48,7 +49,8 @@ const ( ) type Options struct { - PR string + PR string + PRPicker bool } type prDiffCacheEntry struct { @@ -66,6 +68,16 @@ type filesLoadedMsg struct { err error } +type prsLoadedMsg struct { + items []githubpr.Summary + err error +} + +type prResolvedMsg struct { + ctx githubpr.Context + err error +} + type diffLoadedMsg struct { path string rows []diffview.DiffRow @@ -121,6 +133,11 @@ type Model struct { prSvc githubpr.Service prCtx *githubpr.Context prDiffs map[string]prDiffCacheEntry + prPicker bool + prItems []githubpr.Summary + prCursor int + prScroll int + loadingPRs bool width int height int @@ -226,6 +243,7 @@ func NewModelWithOptions(opts Options) (Model, error) { var prCtx *githubpr.Context prDiffs := make(map[string]prDiffCacheEntry) mode := reviewModeLocal + prPicker := false if strings.TrimSpace(opts.PR) != "" { ctx, err := prSvc.ResolvePR(context.Background(), repoRoot, opts.PR) if err != nil { @@ -233,6 +251,9 @@ func NewModelWithOptions(opts Options) (Model, error) { } prCtx = &ctx mode = reviewModePR + } else if opts.PRPicker { + mode = reviewModePR + prPicker = true } m := Model{ @@ -246,6 +267,7 @@ func NewModelWithOptions(opts Options) (Model, error) { prSvc: prSvc, prCtx: prCtx, prDiffs: prDiffs, + prPicker: prPicker, helpOpen: false, filePaneW: filePaneWidthDefault, treeCollapsed: make(map[string]bool), @@ -275,6 +297,10 @@ func NewModelWithOptions(opts Options) (Model, error) { } func (m Model) Init() tea.Cmd { + if m.reviewMode == reviewModePR && m.prPicker { + m.loadingPRs = true + return tea.Batch(m.loadPRsCmd(), alertTickCmd()) + } m.loadingFiles = true return tea.Batch(m.loadFilesCmd(), alertTickCmd()) } @@ -366,6 +392,34 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } return m, nil + case prsLoadedMsg: + m.loadingPRs = false + m.err = msg.err + m.prItems = msg.items + if m.prCursor < 0 { + m.prCursor = 0 + } + if m.prCursor >= len(m.prItems) { + m.prCursor = len(m.prItems) - 1 + } + if m.prCursor < 0 { + m.prCursor = 0 + } + return m, nil + + case prResolvedMsg: + m.loadingPRs = false + if msg.err != nil { + m.setAlert(fmt.Sprintf("failed to load PR: %v", msg.err)) + return m, nil + } + ctx := msg.ctx + m.prCtx = &ctx + m.prPicker = false + m.prDiffs = make(map[string]prDiffCacheEntry) + m.loadingFiles = true + return m, m.loadFilesCmd() + case clipboardResultMsg: if msg.err != nil { m.setAlert(fmt.Sprintf("export failed: %v", msg.err)) @@ -455,8 +509,25 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } if key.Matches(msg, m.keys.Quit) { + if m.reviewMode == reviewModePR && !m.prPicker && m.prCtx != nil { + m.prPicker = true + m.prCtx = nil + m.prDiffs = make(map[string]prDiffCacheEntry) + m.prCursor = 0 + m.prScroll = 0 + m.focus = focusFiles + m.fileHidden = false + if len(m.prItems) == 0 { + m.loadingPRs = true + return m, m.loadPRsCmd() + } + return m, nil + } return m, tea.Quit } + if m.prPicker { + return m.updatePRPicker(msg) + } if key.Matches(msg, m.keys.ToggleFocus) { if m.focus == focusFiles { m.focus = focusDiff @@ -595,6 +666,184 @@ func (m Model) updateFilesPane(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m, nil } +func (m Model) updatePRPicker(msg tea.KeyMsg) (tea.Model, tea.Cmd) { + if key.Matches(msg, m.keys.Refresh) { + m.loadingPRs = true + return m, m.loadPRsCmd() + } + + if len(m.prItems) == 0 { + return m, nil + } + m.clampPRCursor() + + switch { + case key.Matches(msg, m.keys.Up): + if m.prCursor > 0 { + m.prCursor-- + } + m.ensurePRCursorVisible() + return m, nil + + case key.Matches(msg, m.keys.Down): + if m.prCursor < len(m.prItems)-1 { + m.prCursor++ + } + m.ensurePRCursorVisible() + return m, nil + + case key.Matches(msg, m.keys.ScrollDown): + m.scrollPRWindow(1) + return m, nil + + case key.Matches(msg, m.keys.ScrollUp): + m.scrollPRWindow(-1) + return m, nil + + case key.Matches(msg, m.keys.PageDown): + m.pagePRs(1) + return m, nil + + case key.Matches(msg, m.keys.PageUp): + m.pagePRs(-1) + return m, nil + + case key.Matches(msg, m.keys.Top): + m.prCursor = 0 + m.ensurePRCursorVisible() + return m, nil + + case key.Matches(msg, m.keys.Bottom): + m.prCursor = len(m.prItems) - 1 + m.ensurePRCursorVisible() + return m, nil + + case key.Matches(msg, m.keys.Open): + selected := m.prItems[m.prCursor] + m.loadingPRs = true + return m, m.resolvePRCmd(selected.Number) + } + + return m, nil +} + +func (m *Model) clampPRCursor() { + if len(m.prItems) == 0 { + m.prCursor = 0 + m.prScroll = 0 + return + } + if m.prCursor < 0 { + m.prCursor = 0 + } + if m.prCursor >= len(m.prItems) { + m.prCursor = len(m.prItems) - 1 + } +} + +func (m *Model) prPageSize() int { + if m.height <= 0 { + return 1 + } + footerPlain := truncateLinesToWidth(m.helpText(), m.width) + footerHeight := lineCount(footerPlain) + dockHeight := 0 + if m.alertMsg != "" { + dockHeight = lipgloss.Height(m.renderAlertDock()) + } + paneContentHeight := max(1, m.height-footerHeight-dockHeight-2) + listHeight := paneContentHeight - 2 + if listHeight < 1 { + listHeight = 1 + } + return listHeight +} + +func (m *Model) ensurePRCursorVisible() { + if len(m.prItems) == 0 { + m.prScroll = 0 + return + } + page := m.prPageSize() + if page < 1 { + page = 1 + } + maxScroll := len(m.prItems) - page + if maxScroll < 0 { + maxScroll = 0 + } + if m.prScroll < 0 { + m.prScroll = 0 + } + if m.prScroll > maxScroll { + m.prScroll = maxScroll + } + if m.prCursor < m.prScroll { + m.prScroll = m.prCursor + } + if m.prCursor >= m.prScroll+page { + m.prScroll = m.prCursor - page + 1 + } + if m.prScroll < 0 { + m.prScroll = 0 + } + if m.prScroll > maxScroll { + m.prScroll = maxScroll + } +} + +func (m *Model) scrollPRWindow(delta int) { + if len(m.prItems) == 0 || delta == 0 { + return + } + page := m.prPageSize() + if page < 1 { + page = 1 + } + maxScroll := len(m.prItems) - page + if maxScroll < 0 { + maxScroll = 0 + } + oldTop := m.prScroll + newTop := oldTop + delta + if newTop < 0 { + newTop = 0 + } + if newTop > maxScroll { + newTop = maxScroll + } + if newTop == oldTop { + return + } + rel := m.prCursor - oldTop + if rel < 0 { + rel = 0 + } + if rel >= page { + rel = page - 1 + } + m.prScroll = newTop + m.prCursor = newTop + rel + if m.prCursor >= len(m.prItems) { + m.prCursor = len(m.prItems) - 1 + } +} + +func (m *Model) pagePRs(direction int) { + if len(m.prItems) == 0 || direction == 0 { + return + } + page := m.prPageSize() + if page < 1 { + page = 1 + } + step := page + if step > 1 { + step-- + } + m.scrollPRWindow(direction * step) +} + func (m *Model) updateSelectedFileFromCursor(entries []fileTreeEntry) (tea.Model, tea.Cmd) { m.clampFileCursor(entries) entry := entries[m.fileCursor] @@ -1660,7 +1909,9 @@ func (m Model) View() string { m.refreshDiffContent() content := "" - if m.focus == focusComments { + if m.prPicker { + content = m.renderPRPickerPane(m.width, paneContentHeight) + } else if m.focus == focusComments { content = m.renderCommentsPane(m.width, paneContentHeight) } else { rightPane := m.renderDiffPanes(oldPaneW, newPaneW, paneContentHeight) @@ -1689,6 +1940,14 @@ func (m Model) helpText() string { if m.leaderPending { leaderHint = "leader pending | " } + if m.prPicker { + if !m.helpOpen { + return leaderHint + "PR picker | j/k move | enter open PR | r refresh | q quit | ? help" + } + return strings.Join([]string{ + "PR picker: j/k move, ctrl-e/ctrl-y scroll, ctrl-f/ctrl-b page, g/G top/bottom, enter open PR, r refresh, q quit", + }, "\n") + } modeHint := "" if m.reviewMode == reviewModePR && m.prCtx != nil { modeHint = fmt.Sprintf("PR #%d %s/%s | ", m.prCtx.Number, m.prCtx.Owner, m.prCtx.Repo) @@ -1885,6 +2144,82 @@ func (m Model) renderDockPanel(title string, titleColor, borderColor lipgloss.Co Render(titleBar + "\n" + bodyBlock) } +func (m Model) renderPRPickerPane(width, height int) string { + borderColor := lipgloss.Color("245") + paneStyle := lipgloss.NewStyle(). + Width(max(1, width)). + Height(max(1, height)). + Border(lipgloss.NormalBorder()). + BorderForeground(borderColor) + + title := "Open Pull Requests" + if m.loadingPRs { + title += " (loading...)" + } + + bodyLines := []string{title, ""} + if len(m.prItems) == 0 { + if m.loadingPRs { + bodyLines = append(bodyLines, "Loading open PRs...") + } else { + bodyLines = append(bodyLines, "No open PRs found.") + } + if m.err != nil { + bodyLines = append(bodyLines, "", fmt.Sprintf("error: %v", m.err)) + } + return paneStyle.Render(strings.Join(bodyLines, "\n")) + } + + cursor := m.prCursor + if cursor < 0 { + cursor = 0 + } + if cursor >= len(m.prItems) { + cursor = len(m.prItems) - 1 + } + + pageSize := m.prPageSize() + if pageSize < 1 { + pageSize = 1 + } + maxScroll := len(m.prItems) - pageSize + if maxScroll < 0 { + maxScroll = 0 + } + start := m.prScroll + if start < 0 { + start = 0 + } + if start > maxScroll { + start = maxScroll + } + end := start + pageSize + if end > len(m.prItems) { + end = len(m.prItems) + } + + innerW := max(1, width) + for i := start; i < end; i++ { + pr := m.prItems[i] + prefix := " " + if i == cursor { + prefix = "> " + } + line := fmt.Sprintf("%s#%d %s", prefix, pr.Number, strings.TrimSpace(pr.Title)) + line = ansi.Truncate(line, innerW, "") + style := lipgloss.NewStyle().Width(innerW).MaxWidth(innerW) + if i == cursor { + style = style.Foreground(lipgloss.Color("39")).Bold(true) + } + bodyLines = append(bodyLines, style.Render(line)) + } + + if m.err != nil { + bodyLines = append(bodyLines, "", fmt.Sprintf("error: %v", m.err)) + } + return paneStyle.Render(strings.Join(bodyLines, "\n")) +} + func (m Model) renderFilesPane(width, height int) string { border := lipgloss.NormalBorder() borderColor := lipgloss.Color("245") @@ -2370,6 +2705,25 @@ func (m Model) loadFilesCmd() tea.Cmd { } } +func (m Model) loadPRsCmd() tea.Cmd { + cwd := m.cwd + service := m.prSvc + return func() tea.Msg { + items, err := service.ListOpenPRs(context.Background(), cwd) + return prsLoadedMsg{items: items, err: err} + } +} + +func (m Model) resolvePRCmd(number int) tea.Cmd { + cwd := m.cwd + service := m.prSvc + input := strconv.Itoa(number) + return func() tea.Msg { + ctx, err := service.ResolvePR(context.Background(), cwd, input) + return prResolvedMsg{ctx: ctx, err: err} + } +} + func (m Model) loadCommentStaleCmd(items []gitint.FileItem, commentMap map[string]comments.Comment, mode gitint.DiffMode) tea.Cmd { itemSnapshot := append([]gitint.FileItem(nil), items...) commentSnapshot := make([]comments.Comment, 0, len(commentMap)) diff --git a/internal/app/model_pr_cache_test.go b/internal/app/model_pr_cache_test.go index 89977b6..0ecd56d 100644 --- a/internal/app/model_pr_cache_test.go +++ b/internal/app/model_pr_cache_test.go @@ -22,6 +22,10 @@ func (m *mockPRService) ResolvePR(context.Context, string, string) (githubpr.Con return githubpr.Context{}, nil } +func (m *mockPRService) ListOpenPRs(context.Context, string) ([]githubpr.Summary, error) { + return nil, nil +} + func (m *mockPRService) ListFiles(context.Context, githubpr.Context) ([]git.FileItem, error) { return nil, nil } diff --git a/internal/app/model_pr_picker_test.go b/internal/app/model_pr_picker_test.go new file mode 100644 index 0000000..a3e7f81 --- /dev/null +++ b/internal/app/model_pr_picker_test.go @@ -0,0 +1,85 @@ +package app + +import ( + "context" + "testing" + + tea "github.com/charmbracelet/bubbletea" + + "diffman/internal/comments" + "diffman/internal/git" + "diffman/internal/githubpr" +) + +type pickerPRService struct { + resolved githubpr.Context +} + +func (s *pickerPRService) ListOpenPRs(context.Context, string) ([]githubpr.Summary, error) { + return []githubpr.Summary{{Number: 42, Title: "Test PR"}}, nil +} + +func (s *pickerPRService) ResolvePR(context.Context, string, string) (githubpr.Context, error) { + return s.resolved, nil +} + +func (s *pickerPRService) ListFiles(context.Context, githubpr.Context) ([]git.FileItem, error) { + return nil, nil +} + +func (s *pickerPRService) Diff(context.Context, githubpr.Context, string) (string, error) { + return "", nil +} + +func (s *pickerPRService) SubmitReviewComments(context.Context, githubpr.Context, string, string, []comments.Comment) error { + return nil +} + +func TestQuitFromPRReviewReturnsToPicker(t *testing.T) { + m := Model{ + reviewMode: reviewModePR, + prCtx: &githubpr.Context{Owner: "acme", Repo: "widgets", Number: 7}, + prPicker: false, + prItems: []githubpr.Summary{{Number: 7, Title: "A"}}, + prDiffs: map[string]prDiffCacheEntry{"a.go": {}}, + keys: defaultKeyMap(), + } + + next, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'q'}}) + m2 := next.(Model) + + if !m2.prPicker { + t.Fatalf("expected q to return to PR picker") + } + if m2.prCtx != nil { + t.Fatalf("expected active PR context to be cleared when returning to picker") + } +} + +func TestEnterInPRPickerResolvesAndEntersReview(t *testing.T) { + svc := &pickerPRService{resolved: githubpr.Context{Owner: "acme", Repo: "widgets", Number: 42}} + m := Model{ + reviewMode: reviewModePR, + prPicker: true, + prSvc: svc, + cwd: "/tmp", + prItems: []githubpr.Summary{{Number: 42, Title: "Test PR"}}, + keys: defaultKeyMap(), + } + + modelAfterEnter, cmd := m.Update(tea.KeyMsg{Type: tea.KeyEnter}) + if cmd == nil { + t.Fatalf("expected enter to start PR resolve command") + } + + resolvedMsg := cmd().(prResolvedMsg) + next, _ := modelAfterEnter.(Model).Update(resolvedMsg) + m2 := next.(Model) + + if m2.prPicker { + t.Fatalf("expected picker to close after PR resolve") + } + if m2.prCtx == nil || m2.prCtx.Number != 42 { + t.Fatalf("expected resolved PR context to be active") + } +} diff --git a/internal/githubpr/gh.go b/internal/githubpr/gh.go index 11bce57..4c8712d 100644 --- a/internal/githubpr/gh.go +++ b/internal/githubpr/gh.go @@ -17,6 +17,82 @@ import ( type ghService struct{} +func (ghService) ListOpenPRs(ctx context.Context, cwd string) ([]Summary, error) { + owner, repo, err := discoverGitHubRepo(ctx, cwd) + if err != nil { + return nil, err + } + + body, err := util.Run( + ctx, + "", + "gh", + "api", + "--paginate", + "--slurp", + fmt.Sprintf("repos/%s/%s/pulls?state=open&per_page=100", owner, repo), + ) + if err != nil { + return nil, err + } + + type openPR struct { + Number int `json:"number"` + Title string `json:"title"` + HTMLURL string `json:"html_url"` + Head struct { + SHA string `json:"sha"` + Ref string `json:"ref"` + } `json:"head"` + Base struct { + Ref string `json:"ref"` + } `json:"base"` + } + + parse := func(data []byte) ([]openPR, error) { + var direct []openPR + if err := json.Unmarshal(data, &direct); err == nil { + return direct, nil + } + var pages [][]openPR + if err := json.Unmarshal(data, &pages); err != nil { + return nil, fmt.Errorf("parse open prs: %w", err) + } + total := 0 + for _, page := range pages { + total += len(page) + } + out := make([]openPR, 0, total) + for _, page := range pages { + out = append(out, page...) + } + return out, nil + } + + openPRs, err := parse([]byte(body)) + if err != nil { + return nil, err + } + + out := make([]Summary, 0, len(openPRs)) + for _, pr := range openPRs { + out = append(out, Summary{ + Owner: owner, + Repo: repo, + Number: pr.Number, + Title: pr.Title, + URL: pr.HTMLURL, + HeadSHA: pr.Head.SHA, + HeadRef: pr.Head.Ref, + BaseRef: pr.Base.Ref, + }) + } + sort.Slice(out, func(i, j int) bool { + return out[i].Number > out[j].Number + }) + return out, nil +} + func (ghService) ResolvePR(ctx context.Context, cwd, input string) (Context, error) { owner, repo, number, err := parsePRInput(input) if err != nil { diff --git a/internal/githubpr/service.go b/internal/githubpr/service.go index 036bc7b..6a56122 100644 --- a/internal/githubpr/service.go +++ b/internal/githubpr/service.go @@ -18,7 +18,19 @@ type Context struct { BaseRef string } +type Summary struct { + Owner string + Repo string + Number int + Title string + URL string + HeadSHA string + HeadRef string + BaseRef string +} + type Service interface { + ListOpenPRs(ctx context.Context, cwd string) ([]Summary, error) ResolvePR(ctx context.Context, cwd, input string) (Context, error) ListFiles(ctx context.Context, pr Context) ([]gitint.FileItem, error) Diff(ctx context.Context, pr Context, path string) (string, error)