diff --git a/.demo/demo.gif b/.demo/demo.gif index 2575fe5..6290607 100644 Binary files a/.demo/demo.gif and b/.demo/demo.gif differ diff --git a/.demo/demo.mp4 b/.demo/demo.mp4 index bba608d..ea5e7fc 100644 Binary files a/.demo/demo.mp4 and b/.demo/demo.mp4 differ diff --git a/.demo/demo.tape b/.demo/demo.tape index 5942082..6522133 100644 --- a/.demo/demo.tape +++ b/.demo/demo.tape @@ -27,6 +27,7 @@ Output .demo/demo.mp4 Set Shell "zsh" Set FontFamily "JetBrainsMono Nerd Font Mono" +Set Padding 0 Set FontSize 13 Set Width 1280 Set Height 760 diff --git a/.demo/fake-sidecar/fixtures.go b/.demo/fake-sidecar/fixtures.go index c2da473..da11015 100644 --- a/.demo/fake-sidecar/fixtures.go +++ b/.demo/fake-sidecar/fixtures.go @@ -21,22 +21,21 @@ const ( other = "octocat" // the third user with a pre-existing comment ) -// the two changed files, base and head. theme.lua flips monokai -> dracula, widens -// context, and defaults the theme in setup() (a second, separate hunk to land a fresh -// review comment on); palette.lua gains the accent colour the theme references. -var fileVersions = map[string][2]string{ - "lua/theme.lua": { - // base - "local M = {}\n\nM.theme = \"monokai\"\nM.context = 10\n\nfunction M.setup()\n return M.theme\nend\n\nreturn M\n", - // head - "local M = {}\n\nM.theme = \"dracula\"\nM.context = 20\nM.accent = \"#bd93f9\"\n\nfunction M.setup()\n M.theme = M.theme or \"dracula\"\n return M.theme\nend\n\nreturn M\n", - }, - "lua/palette.lua": { - // base - "local M = {}\n\nM.colours = {\n red = \"#ff5555\",\n green = \"#50fa7b\",\n}\n\nreturn M\n", - // head - "local M = {}\n\nM.colours = {\n red = \"#ff5555\",\n green = \"#50fa7b\",\n purple = \"#bd93f9\",\n}\n\nreturn M\n", - }, +// the changed files as they exist at each ref, keyed by their path at that ref. theme.lua +// flips monokai -> dracula, widens context, and defaults the theme in setup() (a second, +// separate hunk to land a fresh review comment on); palette.lua gains the accent colour +// the theme references. monokai.lua -> dracula.lua is the rename: it exists under one +// name at the base and the other at the head, which is all github records of a move. +var baseBlobs = map[string]string{ + "lua/theme.lua": "local M = {}\n\nM.theme = \"monokai\"\nM.context = 10\n\nfunction M.setup()\n return M.theme\nend\n\nreturn M\n", + "lua/palette.lua": "local M = {}\n\nM.colours = {\n red = \"#ff5555\",\n green = \"#50fa7b\",\n}\n\nreturn M\n", + "lua/monokai.lua": "local M = {}\n\nM.name = \"monokai\"\nM.bg = \"#272822\"\nM.fg = \"#f8f8f2\"\nM.comment = \"#75715e\"\n\nreturn M\n", +} + +var headBlobs = map[string]string{ + "lua/theme.lua": "local M = {}\n\nM.theme = \"dracula\"\nM.context = 20\nM.accent = \"#bd93f9\"\n\nfunction M.setup()\n M.theme = M.theme or \"dracula\"\n return M.theme\nend\n\nreturn M\n", + "lua/palette.lua": "local M = {}\n\nM.colours = {\n red = \"#ff5555\",\n green = \"#50fa7b\",\n purple = \"#bd93f9\",\n}\n\nreturn M\n", + "lua/dracula.lua": "local M = {}\n\nM.name = \"dracula\"\nM.bg = \"#282a36\"\nM.fg = \"#f8f8f2\"\nM.comment = \"#6272a4\"\n\nreturn M\n", } // fixture is the in-memory github stand-in. it satisfies handlers.API and keeps the @@ -108,6 +107,7 @@ func (f *fixture) GetPR(_ context.Context, _, _ string, _ int) (*github.PRDetail Files: []github.PRFile{ {Path: "lua/theme.lua", Status: "modified", Additions: 4, Deletions: 2, ViewedState: f.viewedState("lua/theme.lua")}, {Path: "lua/palette.lua", Status: "modified", Additions: 1, Deletions: 0, ViewedState: f.viewedState("lua/palette.lua")}, + {Path: "lua/dracula.lua", Status: "renamed", PreviousPath: "lua/monokai.lua", Additions: 3, Deletions: 3, ViewedState: f.viewedState("lua/dracula.lua")}, }, }, nil } @@ -120,17 +120,26 @@ func (f *fixture) viewedState(path string) string { return "UNVIEWED" } -func (f *fixture) GetFileVersions(_ context.Context, _, _ string, _ int, path, _, _ string) (*github.FileVersions, error) { - v, ok := fileVersions[path] - if !ok { - return &github.FileVersions{Base: github.FileBlob{Missing: true}, Head: github.FileBlob{Missing: true}}, nil +func (f *fixture) GetFileVersions(_ context.Context, _, _ string, _ int, path, basePath, _, _ string) (*github.FileVersions, error) { + if basePath == "" { + basePath = path } return &github.FileVersions{ - Base: github.FileBlob{Content: v[0]}, - Head: github.FileBlob{Content: v[1]}, + Base: blobAt(baseBlobs, basePath), + Head: blobAt(headBlobs, path), }, nil } +// blobAt stands in for the Contents API: a path absent at that ref reads as missing, +// which is how an add, a delete and a rename's two half-present paths all render. +func blobAt(blobs map[string]string, path string) github.FileBlob { + content, ok := blobs[path] + if !ok { + return github.FileBlob{Missing: true} + } + return github.FileBlob{Content: content} +} + func (f *fixture) HeadSHA(_ context.Context, _, _ string, _ int) (string, error) { return headSHA, nil } diff --git a/CHANGELOG.md b/CHANGELOG.md index 9970b48..baf4006 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed + +- A renamed file in a PR review showed every line as new, which wasn't very useful. It now diffs against the file's old path, showing only what actually changed + +## [0.1.40] — 2026-09-06 + ### Added - `:checkhealth differ` warns when `status.renames` and `diff.renames` disagree, which is what leaves the file panel listing a rename as an add and a delete where a rev-pair diff lists it as a rename diff --git a/internal/github/cache_test.go b/internal/github/cache_test.go index 808dde2..24d05c3 100644 --- a/internal/github/cache_test.go +++ b/internal/github/cache_test.go @@ -27,7 +27,7 @@ func TestBlobCacheServesContents(t *testing.T) { return nil, nil }) for range 2 { - if _, err := c.GetFileVersions(context.Background(), "o", "r", 3, "a.go", "", ""); err != nil { + if _, err := c.GetFileVersions(context.Background(), "o", "r", 3, "a.go", "", "", ""); err != nil { t.Fatal(err) } } @@ -49,11 +49,11 @@ func TestClearCacheFlushesBlobs(t *testing.T) { return resp(200, `{"base":{"sha":"BASE"},"head":{"sha":"HEAD"}}`, nil), nil }) ctx := context.Background() - if _, err := c.GetFileVersions(ctx, "o", "r", 3, "a.go", "", ""); err != nil { + if _, err := c.GetFileVersions(ctx, "o", "r", 3, "a.go", "", "", ""); err != nil { t.Fatal(err) } c.ClearCache() - if _, err := c.GetFileVersions(ctx, "o", "r", 3, "a.go", "", ""); err != nil { + if _, err := c.GetFileVersions(ctx, "o", "r", 3, "a.go", "", "", ""); err != nil { t.Fatal(err) } if contentCalls.Load() != 4 { diff --git a/internal/github/files.go b/internal/github/files.go index f4d051f..5a7008a 100644 --- a/internal/github/files.go +++ b/internal/github/files.go @@ -10,13 +10,14 @@ import ( "github.com/undont/differ.nvim/internal/protocol" ) -// GetFileVersions returns the full base and head blobs for one path in a PR. the -// caller passes the pinned base/head shas (from get_pr) so the prRefs round-trip is -// skipped and the exact session blobs are fetched; an empty sha falls back to -// resolving it. the two sides are fetched concurrently (independent immutable blobs, -// cache is mutex-guarded for fan-out). a 404 on a side marks it missing (an added -// file has no base, a deleted file has no head) rather than failing the call. -func (c *Client) GetFileVersions(ctx context.Context, owner, repo string, number int, path, base, head string) (*FileVersions, error) { +// GetFileVersions returns the full base and head blobs for one file in a PR. basePath is +// the file's path at the base sha: a rename's previous_path from the REST file list, else +// path. base and head are the caller's pinned shas (from get_pr), which skip the prRefs +// round-trip and pin the exact session blobs; empty resolves them. +func (c *Client) GetFileVersions(ctx context.Context, owner, repo string, number int, path, basePath, base, head string) (*FileVersions, error) { + if basePath == "" { + basePath = path + } if base == "" || head == "" { rbase, rhead, err := c.prRefs(ctx, owner, repo, number) if err != nil { @@ -31,7 +32,7 @@ func (c *Client) GetFileVersions(ctx context.Context, owner, repo string, number } baseCh := make(chan blobResult, 1) go func() { - b, err := c.rawBlob(ctx, owner, repo, path, base) + b, err := c.rawBlob(ctx, owner, repo, basePath, base) baseCh <- blobResult{b, err} }() headBlob, headErr := c.rawBlob(ctx, owner, repo, path, head) diff --git a/internal/github/read_test.go b/internal/github/read_test.go index bb22101..766f861 100644 --- a/internal/github/read_test.go +++ b/internal/github/read_test.go @@ -28,7 +28,7 @@ func TestGetFileVersions(t *testing.T) { t.Fatalf("unexpected request %s %s", r.Method, r.URL.Path) return nil, nil }) - fv, err := c.GetFileVersions(context.Background(), "o", "r", 3, "dir/a.go", "", "") + fv, err := c.GetFileVersions(context.Background(), "o", "r", 3, "dir/a.go", "", "", "") if err != nil { t.Fatal(err) } @@ -71,7 +71,7 @@ func TestGetFileVersionsPinnedRefsSkipPrRefs(t *testing.T) { t.Fatalf("unexpected request %s %s", r.Method, r.URL.Path) return nil, nil }) - fv, err := c.GetFileVersions(context.Background(), "o", "r", 3, "a.go", "pinbase", "pinhead") + fv, err := c.GetFileVersions(context.Background(), "o", "r", 3, "a.go", "", "pinbase", "pinhead") if err != nil { t.Fatal(err) } @@ -94,7 +94,7 @@ func TestGetFileVersionsAddedFileMissingBase(t *testing.T) { t.Fatalf("unexpected request %s", r.URL.Path) return nil, nil }) - fv, err := c.GetFileVersions(context.Background(), "o", "r", 3, "new.go", "", "") + fv, err := c.GetFileVersions(context.Background(), "o", "r", 3, "new.go", "", "", "") if err != nil { t.Fatal(err) } @@ -106,6 +106,41 @@ func TestGetFileVersionsAddedFileMissingBase(t *testing.T) { } } +// a rename's two sides live at different paths: the base blob is fetched at basePath +// (the REST file list's previous_path), the head blob at path. asking for either path +// at the other side's ref is the bug this guards. +func TestGetFileVersionsRenameFetchesBothPaths(t *testing.T) { + c := newClient(func(r *http.Request) (*http.Response, error) { + ref := r.URL.Query().Get("ref") + switch { + case strings.HasSuffix(r.URL.Path, "/pulls/3"): + return resp(200, prRefsREST, nil), nil + case strings.Contains(r.URL.Path, "/contents/old/a.go"): + if ref != "basesha" { + t.Errorf("old path should only be asked for at the base, got ref %q", ref) + } + return resp(200, "old content", nil), nil + case strings.Contains(r.URL.Path, "/contents/new/b.go"): + if ref != "headsha" { + t.Errorf("new path should only be asked for at the head, got ref %q", ref) + } + return resp(200, "new content", nil), nil + } + t.Fatalf("unexpected request %s", r.URL.Path) + return nil, nil + }) + fv, err := c.GetFileVersions(context.Background(), "o", "r", 3, "new/b.go", "old/a.go", "", "") + if err != nil { + t.Fatal(err) + } + if fv.Base.Missing || fv.Base.Content != "old content" { + t.Errorf("base should come from the previous path, not be missing: %+v", fv.Base) + } + if fv.Head.Missing || fv.Head.Content != "new content" { + t.Errorf("head should come from the new path: %+v", fv.Head) + } +} + func TestGetFileVersionsPropagatesError(t *testing.T) { c := newClient(func(r *http.Request) (*http.Response, error) { if strings.HasSuffix(r.URL.Path, "/pulls/3") { @@ -113,7 +148,7 @@ func TestGetFileVersionsPropagatesError(t *testing.T) { } return resp(401, `{"message":"Bad credentials"}`, nil), nil }) - _, err := c.GetFileVersions(context.Background(), "o", "r", 3, "a.go", "", "") + _, err := c.GetFileVersions(context.Background(), "o", "r", 3, "a.go", "", "", "") if codeOf(t, err) != protocol.CodeAuth { t.Fatalf("want auth, got %v", err) } diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 86a3cbe..6386ff6 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -25,7 +25,7 @@ type Registry map[string]Handler type API interface { ListPRs(ctx context.Context, owner, repo, filter string) ([]github.PR, error) GetPR(ctx context.Context, owner, repo string, number int) (*github.PRDetail, error) - GetFileVersions(ctx context.Context, owner, repo string, number int, path, base, head string) (*github.FileVersions, error) + GetFileVersions(ctx context.Context, owner, repo string, number int, path, basePath, base, head string) (*github.FileVersions, error) HeadSHA(ctx context.Context, owner, repo string, number int) (string, error) GetThreads(ctx context.Context, owner, repo string, number int) ([]github.Thread, error) GetTimeline(ctx context.Context, owner, repo string, number int) (*github.Timeline, error) diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 00b0a52..9853dba 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -22,6 +22,7 @@ type mockAPI struct { gotFilter string gotNumber int gotPath string + gotBasePath string gotBase string gotHead string gotReviewID string @@ -54,10 +55,11 @@ func (m *mockAPI) GetPR(_ context.Context, _, _ string, number int) (*github.PRD return m.detail, nil } -func (m *mockAPI) GetFileVersions(_ context.Context, _, _ string, number int, path, base, head string) (*github.FileVersions, error) { +func (m *mockAPI) GetFileVersions(_ context.Context, _, _ string, number int, path, basePath, base, head string) (*github.FileVersions, error) { m.called = true m.gotNumber = number m.gotPath = path + m.gotBasePath = basePath m.gotBase = base m.gotHead = head return &github.FileVersions{}, nil @@ -236,13 +238,16 @@ func TestMalformedParams(t *testing.T) { func TestGetFileVersionsRoutes(t *testing.T) { m := &mockAPI{} - _, err := deps(m).getFileVersions(context.Background(), json.RawMessage(`{"owner":"o","repo":"r","number":7,"path":"a.go","base":"B","head":"H"}`)) + _, err := deps(m).getFileVersions(context.Background(), json.RawMessage(`{"owner":"o","repo":"r","number":7,"path":"a.go","previous_path":"old.go","base":"B","head":"H"}`)) if err != nil { t.Fatal(err) } if m.gotNumber != 7 || m.gotPath != "a.go" { t.Errorf("params not forwarded: number=%d path=%q", m.gotNumber, m.gotPath) } + if m.gotBasePath != "old.go" { + t.Errorf("rename base path not forwarded: %q", m.gotBasePath) + } if m.gotBase != "B" || m.gotHead != "H" { t.Errorf("pinned refs not forwarded: base=%q head=%q", m.gotBase, m.gotHead) } diff --git a/internal/handlers/read_files.go b/internal/handlers/read_files.go index cb480da..badc15e 100644 --- a/internal/handlers/read_files.go +++ b/internal/handlers/read_files.go @@ -10,6 +10,8 @@ import ( type getFileVersionsParams struct { prParams Path string `json:"path"` + // the file's path at the base sha: a rename's previous_path, else absent + PreviousPath string `json:"previous_path"` // optional pinned refs: when the client already holds the PR's base/head shas // (from get_pr), it sends them so the sidecar skips the prRefs round-trip and // fetches the exact blobs the session is reviewing. empty falls back to prRefs @@ -29,5 +31,5 @@ func (d Deps) getFileVersions(ctx context.Context, params json.RawMessage) (any, if p.Path == "" { return nil, protocol.NewError(protocol.CodeBadRequest, "path is required") } - return d.GH.GetFileVersions(ctx, p.Owner, p.Repo, p.Number, p.Path, p.Base, p.Head) + return d.GH.GetFileVersions(ctx, p.Owner, p.Repo, p.Number, p.Path, p.PreviousPath, p.Base, p.Head) } diff --git a/lua/differ/pr/client.lua b/lua/differ/pr/client.lua index e922209..a6d8cdf 100644 --- a/lua/differ/pr/client.lua +++ b/lua/differ/pr/client.lua @@ -30,20 +30,22 @@ function M.get_pr(pr, cb) end -- get_file_versions result: {base: {content, missing?}, head: {content, missing?}, --- truncated?}. the sidecar fetches `path` at both the base and head refs. pass the --- pinned `refs` ({base, head} from get_pr) so the sidecar skips the prRefs round-trip --- and fetches the exact session blobs; omit to let the sidecar resolve them +-- truncated?}. the sidecar fetches `path` at the head ref and `previous_path` (a +-- rename's old path, else `path`) at the base ref. pass the pinned `refs` ({base, head} +-- from get_pr) so the sidecar skips the prRefs round-trip and fetches the exact session +-- blobs; omit to let the sidecar resolve them ---@param pr { owner: string, repo: string, number: integer } ----@param path string +---@param file { path: string, previous_path?: string } -- only the path(s) are read ---@param refs { base?: string, head?: string }|nil ---@param cb fun(err: table|nil, result: any) -function M.get_file_versions(pr, path, refs, cb) +function M.get_file_versions(pr, file, refs, cb) refs = refs or {} sidecar.request("get_file_versions", { owner = pr.owner, repo = pr.repo, number = pr.number, - path = path, + path = file.path, + previous_path = file.previous_path, base = refs.base, head = refs.head, }, cb) diff --git a/lua/differ/pr/init.lua b/lua/differ/pr/init.lua index 9f0282e..c8f0e6f 100644 --- a/lua/differ/pr/init.lua +++ b/lua/differ/pr/init.lua @@ -174,7 +174,7 @@ local function prefetch_around(entry) local path = nb.path s.prefetching[path] = true local refs = { base = s.pr_meta.base_sha, head = s.pr_meta.head_sha } - client.get_file_versions(s.pr, path, refs, function(err, vers) + client.get_file_versions(s.pr, nb, refs, function(err, vers) if not guard.owns(s) then return -- session torn down (or replaced) while it was in flight end @@ -258,12 +258,10 @@ local function show_file(entry, focus_line) if cached then return render(cached) end - -- pass entry.path, not previous_path: the sidecar fetches one path at both refs, - -- so a rename shows its head content as an add (true rename diffing is a later - -- sidecar concern; this keeps open-and-navigate correct for the common cases). - -- the pinned shas skip the sidecar's prRefs round-trip (latency discipline) + -- the pinned shas skip the sidecar's prRefs round-trip (latency discipline); the + -- entry rides along whole so a rename's base side is read from its previous_path local refs = { base = session.pr_meta.base_sha, head = session.pr_meta.head_sha } - client.get_file_versions(session.pr, entry.path, refs, function(err, vers) + client.get_file_versions(session.pr, entry, refs, function(err, vers) if not guard.owns(s) then return -- session torn down (or replaced) while the blob was in flight end