diff --git a/README.md b/README.md index 5b1dd05..d22dd53 100644 --- a/README.md +++ b/README.md @@ -219,6 +219,7 @@ own right and leaves the group entry alone, exactly as the web UI does. |---|---| | `D` | from a pull request, open its diff | | `^u` / `^d` | scroll the file's diff | +| `enter` | open the whole file | The changed files are the list and the selected file's diff is the pane beside it, as a unified diff: additions green, deletions red, three lines of context. @@ -229,6 +230,13 @@ A binary file, or one over 256 KiB, says what it is instead of being rendered. A renamed file reads as a new one: the change entry names only where the file landed, so its previous content is at a path boardwalk cannot ask for. +`enter` opens the selected file whole — the change marked in the gutter +rather than colouring the code, so it reads as source. `d` takes the marks +away and leaves the file. Syntax highlighting comes from the file's name, and +a language boardwalk has no lexer for is shown plainly rather than guessed +at. Lines the pull request removed sit where they were, dimmed, and go away +with `d`. + Review comments sit under the line they were written against, marked resolved or unresolved, and a file with discussion on it carries the count in the list beside its name. A comment whose line the diff's three lines of context never @@ -253,6 +261,14 @@ only source at all if your token cannot read your organisation's identities. A tenant where that is locked down behaves exactly as boardwalk did before any of this. See Setup. +### File + +| key | | +|---|---| +| `j` / `k` | move the line cursor | +| `g` / `G` | top / bottom | +| `d` | show / hide the change marks | + ### Session | key | | diff --git a/go.mod b/go.mod index b0f8650..b44e17c 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.27.1 require ( github.com/JohannesKaufmann/html-to-markdown v1.6.0 + github.com/alecthomas/chroma/v2 v2.20.0 github.com/aymanbagabas/go-udiff v0.4.1 github.com/charmbracelet/bubbles v1.0.0 github.com/charmbracelet/bubbletea v1.3.10 @@ -15,7 +16,6 @@ require ( require ( github.com/PuerkitoBio/goquery v1.9.2 // indirect - github.com/alecthomas/chroma/v2 v2.20.0 // indirect github.com/andybalholm/cascadia v1.3.2 // indirect github.com/atotto/clipboard v0.1.4 // indirect github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect diff --git a/internal/ui/composefile.go b/internal/ui/composefile.go new file mode 100644 index 0000000..ee1c9cb --- /dev/null +++ b/internal/ui/composefile.go @@ -0,0 +1,149 @@ +package ui + +import ( + "strings" + + "github.com/aymanbagabas/go-udiff" +) + +// fileLine is one row of the file view: a line of the file, or a line the +// pull request removed, shown where it used to be. +// +// Both numbers are kept because the two sides of a diff advance at different +// rates. New is what a review comment anchors to and what the cursor reports; +// Old is what a removed line is known by, since it no longer exists in the +// new file to have a number there. Whichever side a row does not belong to +// is zero, which no real line number ever is. +type fileLine struct { + Text string + New int + Old int + Kind udiff.OpKind +} + +// composeFile lays the pull request's changes over the whole file. +// +// The diff pane shows hunks: the changed lines and three lines either side. +// This shows the file, with the changes marked in place — which is the point +// of the view, since the reason a change is wrong is usually in the part of +// the file the hunk does not reach. +// +// text is the new side of the file, or the old side for a file the pull +// request deleted, in which case it is empty and every row comes from the +// hunks. Removed lines are spliced in at the new-side position they were +// removed from, so a rewrite reads as a rewrite rather than as an unexplained +// pair of additions. +// +// It is a pure function over the text and the hunks, and deliberately knows +// nothing about rendering: a row numbered from the wrong side puts a "+" +// against code nobody touched, which looks entirely plausible on screen and +// can only be caught by asserting the numbers directly. +func composeFile(text string, hunks []*udiff.Hunk) []fileLine { + lines := splitLines(text) + + // Both maps are keyed by new-side line number. inserted marks the lines + // the pull request added; removed holds what it took out, keyed by the + // line it sat in front of. + inserted := map[int]bool{} + removed := map[int][]fileLine{} + // oldOf carries the old-side number of the unchanged lines inside a + // hunk, where the diff states it outright. Outside the hunks it has to + // be derived — see shifts below. + oldOf := map[int]int{} + // shifts is how far the old side trails the new one after each hunk: + // old = new + delta. A hunk that adds two lines and removes one leaves + // every later line of the new file one number ahead of its old self, and + // a view that assumed the two stayed level would number most of the file + // wrongly the moment anything changed. + type shift struct{ afterNew, delta int } + var shifts []shift + delta := 0 + // after the last line of the new file there is nothing to splice in + // front of, so trailing removals are collected separately. + var trailing []fileLine + + for _, h := range hunks { + positions := diffLineNumbers(h) + lastNew := 0 + for i, l := range h.Lines { + pos := positions[i] + if pos.right > lastNew { + lastNew = pos.right + } + switch l.Kind { + case udiff.Equal: + oldOf[pos.right] = pos.left + case udiff.Insert: + delta-- + inserted[pos.right] = true + case udiff.Delete: + delta++ + row := fileLine{Text: trimNewline(l.Content), Old: pos.left, Kind: udiff.Delete} + // A deletion's own right-hand position is zero — it is not in + // the new file — so it belongs in front of whichever new line + // follows it, which is the one the sides were level at. + switch next := nextRight(h, positions, i); { + case next == 0 || next > len(lines): + trailing = append(trailing, row) + default: + removed[next] = append(removed[next], row) + } + } + } + shifts = append(shifts, shift{afterNew: lastNew, delta: delta}) + } + + out := make([]fileLine, 0, len(lines)+len(trailing)) + next, running := 0, 0 + for i, text := range lines { + n := i + 1 + out = append(out, removed[n]...) + + // Every hunk the walk has now passed contributes its shift to the + // lines beyond it. + for next < len(shifts) && shifts[next].afterNew < n { + running = shifts[next].delta + next++ + } + + kind := udiff.Equal + if inserted[n] { + kind = udiff.Insert + } + row := fileLine{Text: text, New: n, Kind: kind} + if kind == udiff.Equal { + if old, ok := oldOf[n]; ok { + row.Old = old + } else { + row.Old = n + running + } + } + out = append(out, row) + } + return append(out, trailing...) +} + +// nextRight is the new-side line number of the first row after i that has +// one, which is where a deletion at i belongs. Zero when the hunk ends +// without another. +func nextRight(h *udiff.Hunk, positions []diffLine, i int) int { + for j := i + 1; j < len(h.Lines); j++ { + if positions[j].right != 0 { + return positions[j].right + } + } + return 0 +} + +// splitLines breaks a file into its lines without inventing a last empty one +// for the trailing newline nearly every file ends with. +func splitLines(text string) []string { + if text == "" { + return nil + } + return strings.Split(strings.TrimSuffix(text, "\n"), "\n") +} + +// trimNewline drops the newline a hunk line was split on. A line without one +// is the last line of a file that does not end with one. +func trimNewline(s string) string { return strings.TrimSuffix(s, "\n") } diff --git a/internal/ui/composefile_test.go b/internal/ui/composefile_test.go new file mode 100644 index 0000000..75af108 --- /dev/null +++ b/internal/ui/composefile_test.go @@ -0,0 +1,190 @@ +package ui + +import ( + "fmt" + "strings" + "testing" + + "github.com/aymanbagabas/go-udiff" +) + +// show renders a composed file the way an assertion reads best: one line per +// row, gutter and both numbers, so a wrong number is visible in the failure +// rather than having to be worked out from it. +func show(lines []fileLine) string { + var b strings.Builder + for _, l := range lines { + mark := " " + switch l.Kind { + case udiff.Insert: + mark = "+" + case udiff.Delete: + mark = "-" + } + old, new := " ", " " + if l.Old > 0 { + old = fmt.Sprintf("%2d", l.Old) + } + if l.New > 0 { + new = fmt.Sprintf("%2d", l.New) + } + fmt.Fprintf(&b, "%s %s %s %s\n", mark, old, new, l.Text) + } + return b.String() +} + +// TestComposeFileWithNoHunks — a file the pull request lists as changed but +// whose text is identical at both commits, and the plain case underneath +// every other: every line is context, numbered from one. +func TestComposeFileWithNoHunks(t *testing.T) { + got := composeFile("alpha\nbeta\ngamma\n", nil) + + want := ` 1 1 alpha + 2 2 beta + 3 3 gamma +` + if show(got) != want { + t.Errorf("composed:\n%s\nwant:\n%s", show(got), want) + } +} + +// TestComposeFilePlacesInsertionsAndDeletions is the one that matters. The +// hunk covers new lines 2 to 4 and removes an old line 2 while adding two: the +// two sides advance at different rates, and a row numbered from the wrong one +// puts a + against code nobody touched. +func TestComposeFilePlacesInsertionsAndDeletions(t *testing.T) { + text := "func tail() {\n\tdefer close(ch)\n\tgo poll(ctx)\n}\n" + hunks := []*udiff.Hunk{hunk(1, 1, + line(udiff.Equal, "func tail() {\n"), + line(udiff.Delete, "\tgo poll()\n"), + line(udiff.Insert, "\tdefer close(ch)\n"), + line(udiff.Insert, "\tgo poll(ctx)\n"), + line(udiff.Equal, "}\n"), + )} + + want := ` 1 1 func tail() { +- 2 go poll() ++ 2 defer close(ch) ++ 3 go poll(ctx) + 3 4 } +` + if got := show(composeFile(text, hunks)); got != want { + t.Errorf("composed:\n%s\nwant:\n%s", got, want) + } +} + +// TestComposeFileKeepsUntouchedLinesOutsideTheHunks — the whole point of a +// full-file view. The hunk covers three lines in the middle; everything +// before and after it is still the file and still numbered correctly. +func TestComposeFileKeepsUntouchedLinesOutsideTheHunks(t *testing.T) { + text := "one\ntwo\nCHANGED\nfour\nfive\n" + hunks := []*udiff.Hunk{hunk(3, 3, + line(udiff.Delete, "three\n"), + line(udiff.Insert, "CHANGED\n"), + )} + + want := ` 1 1 one + 2 2 two +- 3 three ++ 3 CHANGED + 4 4 four + 5 5 five +` + if got := show(composeFile(text, hunks)); got != want { + t.Errorf("composed:\n%s\nwant:\n%s", got, want) + } +} + +// TestComposeFileWithTwoHunks — the numbering has to survive a gap. The +// second hunk's new-side start is what says where it lands, and drifting by +// the first hunk's net change is the classic way to get this wrong. +func TestComposeFileWithTwoHunks(t *testing.T) { + text := "A+\nb\nc\nd\ne\nF+\n" + hunks := []*udiff.Hunk{ + hunk(1, 1, line(udiff.Insert, "A+\n")), + hunk(5, 6, line(udiff.Insert, "F+\n")), + } + + want := `+ 1 A+ + 1 2 b + 2 3 c + 3 4 d + 4 5 e ++ 6 F+ +` + if got := show(composeFile(text, hunks)); got != want { + t.Errorf("composed:\n%s\nwant:\n%s", got, want) + } +} + +// TestComposeFileWithADeletionAtTheEnd — a deletion after the last surviving +// line has no following line to splice in front of, and must not be dropped. +func TestComposeFileWithADeletionAtTheEnd(t *testing.T) { + text := "kept\n" + hunks := []*udiff.Hunk{hunk(1, 1, + line(udiff.Equal, "kept\n"), + line(udiff.Delete, "gone\n"), + )} + + want := ` 1 1 kept +- 2 gone +` + if got := show(composeFile(text, hunks)); got != want { + t.Errorf("composed:\n%s\nwant:\n%s", got, want) + } +} + +// TestComposeFileOfADeletedFile — no new side at all. Every line is a +// removal, numbered on the old side only. +func TestComposeFileOfADeletedFile(t *testing.T) { + hunks := []*udiff.Hunk{hunk(1, 1, + line(udiff.Delete, "one\n"), + line(udiff.Delete, "two\n"), + )} + + want := `- 1 one +- 2 two +` + if got := show(composeFile("", hunks)); got != want { + t.Errorf("composed:\n%s\nwant:\n%s", got, want) + } +} + +// TestComposeFileWithNoTrailingNewline — a last line without one must not +// become a phantom empty row at the bottom of every file. +func TestComposeFileWithNoTrailingNewline(t *testing.T) { + got := composeFile("alpha\nbeta", nil) + if len(got) != 2 { + t.Fatalf("composed %d lines, want 2:\n%s", len(got), show(got)) + } + if got[1].Text != "beta" { + t.Errorf("last line = %q, want %q", got[1].Text, "beta") + } +} + +func TestComposeFileOfAnEmptyFile(t *testing.T) { + if got := composeFile("", nil); len(got) != 0 { + t.Errorf("composed %d lines for an empty file, want none:\n%s", len(got), show(got)) + } +} + +// TestComposeFileNumbersEveryNewLine — the line cursor reports the number +// inline feedback anchors to, so every line of the new file must carry one. +func TestComposeFileNumbersEveryNewLine(t *testing.T) { + text := "a\nb\nc\nd\n" + hunks := []*udiff.Hunk{hunk(2, 2, line(udiff.Insert, "b\n"))} + + n := 0 + for _, l := range composeFile(text, hunks) { + if l.Kind == udiff.Delete { + continue + } + n++ + if l.New != n { + t.Errorf("row %d carries new-side number %d, want %d", n, l.New, n) + } + } + if n != 4 { + t.Errorf("%d rows belong to the new file, want 4", n) + } +} diff --git a/internal/ui/fileview.go b/internal/ui/fileview.go new file mode 100644 index 0000000..8340fde --- /dev/null +++ b/internal/ui/fileview.go @@ -0,0 +1,296 @@ +package ui + +import ( + "fmt" + "strings" + + "github.com/JacobAtchley/boardwalk/internal/azdo" + "github.com/aymanbagabas/go-udiff" + "github.com/charmbracelet/bubbles/help" + "github.com/charmbracelet/bubbles/key" + "github.com/charmbracelet/bubbles/viewport" + tea "github.com/charmbracelet/bubbletea" +) + +// FileView is one changed file, whole, with the pull request's changes marked +// in place. +// +// The diff pane beside the file list shows hunks — the changed lines and +// three either side — which is the right amount of code to skim a change by +// and the wrong amount to judge one by. The reason a change is wrong is +// usually in the part of the file the hunk does not reach: the caller two +// hundred lines up, the early return it now skips. This is that file. +// +// It is syntax highlighted rather than coloured by change, and the change +// goes in the gutter. Three lines of context are not worth reading as code; +// a whole file is. +type FileView struct { + client *azdo.Client + pr azdo.PullRequest + path string + + // lines is the composed file — every line of it, with the removed ones + // spliced back in where they were. It does not change; showDiff decides + // which of them are rendered. + lines []fileLine + // highlighted is the new side's lines after chroma, indexed by new-side + // line number minus one. Removed lines are not in it: they are old + // content, dimmed rather than coloured, so the eye skips them. + highlighted []string + + threads []azdo.Thread + + // showDiff is whether the change is marked at all. With it off this is + // simply the file, which is what reading the code rather than the change + // wants. + showDiff bool + + // cursor indexes lines, and is what a review comment will anchor to. It + // is a line cursor rather than a scroll position because "the line I am + // looking at" is a thing the next feature has to name exactly. + cursor int + viewport viewport.Model + + // renderedAt and renderedDiff are what the pane was last laid out for, + // so it is rebuilt when the width or the toggle changes and not on every + // keystroke — a thousand-line file is re-rendered from scratch each + // time, and the cursor moves a line at a time. + renderedAt int + renderedDiff bool + rendered bool + + status string + failed bool +} + +// NewFileView builds the view over content the diff pane has already +// fetched. It makes no request of its own: buildFileDiff reads both sides of +// the file to diff them, so by the time a row can be opened the text is +// already in hand. +func NewFileView(c *azdo.Client, pr azdo.PullRequest, path, text string, + hunks []*udiff.Hunk, threads []azdo.Thread) *FileView { + return &FileView{ + client: c, + pr: pr, + path: path, + lines: composeFile(text, hunks), + highlighted: highlightLines(text, path), + threads: azdo.ThreadsForFile(threads, path), + showDiff: true, + viewport: viewport.New(0, 0), + } +} + +func (m *FileView) Update(msg tea.Msg) (View, tea.Cmd) { + switch msg := msg.(type) { + case StatusMsg: + m.status, m.failed = msg.Text, msg.Err + return m, nil + + case tea.KeyMsg: + switch { + case key.Matches(msg, keyBack), key.Matches(msg, keyQuit): + return m, func() tea.Msg { return PopMsg{} } + case key.Matches(msg, keyFileDiff): + m.showDiff = !m.showDiff + // The cursor indexes the composed file, which does not change — + // only which of its lines are drawn. Landing on a hidden row is + // handled by the render, which skips it. + m.invalidate() + return m, nil + case key.Matches(msg, keyLineDown): + m.moveCursor(1) + return m, nil + case key.Matches(msg, keyLineUp): + m.moveCursor(-1) + return m, nil + case key.Matches(msg, keyTop): + m.cursor = 0 + m.invalidate() + return m, nil + case key.Matches(msg, keyBottom): + m.cursor = max(0, len(m.lines)-1) + m.invalidate() + return m, nil + } + } + + var cmd tea.Cmd + m.viewport, cmd = m.viewport.Update(msg) + return m, cmd +} + +// moveCursor steps the cursor and stops at either end. +// +// It does not wrap, which the landing menu and the pickers do. Those are +// short lists of alternatives where wrapping saves a keystroke; this is a +// file, and jumping from the last line to the first because a key repeated +// once too often is disorienting rather than convenient. +func (m *FileView) moveCursor(by int) { + if len(m.lines) == 0 { + return + } + m.cursor = min(max(m.cursor+by, 0), len(m.lines)-1) + m.invalidate() +} + +// anchorLine is the new-file line a review comment written here would be +// anchored to, and whether there is one at all. A removed line has none: it +// is not in the file any more, so there is nothing for the server to hang a +// comment on. +func (m *FileView) anchorLine() (int, bool) { + if m.cursor < 0 || m.cursor >= len(m.lines) { + return 0, false + } + row := m.lines[m.cursor] + return row.New, row.New > 0 +} + +func (m *FileView) invalidate() { m.rendered = false } + +func (m *FileView) Body(width, height int) string { + m.viewport.Width, m.viewport.Height = width, height + + if !m.rendered || m.renderedAt != width || m.renderedDiff != m.showDiff { + m.viewport.SetContent(m.render(width)) + m.renderedAt, m.renderedDiff, m.rendered = width, m.showDiff, true + } + m.follow() + return m.viewport.View() +} + +// follow keeps the cursor on screen without moving the pane any further than +// it has to, so paging down a file reads as a page rather than as a jump. +func (m *FileView) follow() { + row := m.screenRow(m.cursor) + switch { + case row < m.viewport.YOffset: + m.viewport.SetYOffset(row) + case row >= m.viewport.YOffset+m.viewport.Height: + m.viewport.SetYOffset(row - m.viewport.Height + 1) + } +} + +// screenRow is which rendered row a line ended up on, which is not its index: +// hidden removals and the review comments written under a line both move +// everything below them down. +func (m *FileView) screenRow(index int) int { + row := 0 + for i, l := range m.lines { + if i == index { + return row + } + if !m.visible(l) { + continue + } + row++ + row += m.commentRows(l) + } + return row +} + +// visible reports whether a composed line is drawn at all. With the diff off +// the removed lines are not: they are not in the file, and the point of +// turning it off is to read the file. +func (m *FileView) visible(l fileLine) bool { + return m.showDiff || l.Kind != udiff.Delete +} + +// commentRows is how many rows the threads under a line take. +func (m *FileView) commentRows(l fileLine) int { + n := 0 + for _, t := range m.threadsAt(l) { + n += 1 + len(t.Comments) + } + return n +} + +// threadsAt is the discussion written against a line. +func (m *FileView) threadsAt(l fileLine) []azdo.Thread { + if l.New == 0 { + return nil + } + var out []azdo.Thread + for _, t := range m.threads { + if t.RightSide && t.Line == l.New { + out = append(out, t) + } + } + return out +} + +func (m *FileView) render(width int) string { + var b strings.Builder + + // The gutter is a mark, both line numbers' width, and two spaces. Sized + // from the file rather than fixed, so a short file does not carry a + // column of blanks for line numbers it will never reach. + digits := len(fmt.Sprint(max(1, len(m.lines)))) + + for _, l := range m.lines { + if !m.visible(l) { + continue + } + m.writeLine(&b, l, digits, width) + for _, t := range m.threadsAt(l) { + writeThread(&b, t, width) + } + } + return b.String() +} + +func (m *FileView) writeLine(b *strings.Builder, l fileLine, digits, width int) { + mark, markStyle := " ", normalRow + number := l.New + if m.showDiff { + switch l.Kind { + case udiff.Insert: + mark, markStyle = "+", addedLine + case udiff.Delete: + mark, markStyle, number = "-", removedLine, l.Old + } + } + + gutter := markStyle.Render(mark) + " " + + chromeStyle.Render(fmt.Sprintf("%*d", digits, number)) + + // A removed line is old content and is dimmed rather than highlighted; + // the highlighted text is indexed by the new file's numbering, which a + // removal has no place in. + text := l.Text + if l.Kind == udiff.Delete { + text = chromeStyle.Render(expandTabs(l.Text)) + } else if i := l.New - 1; i >= 0 && i < len(m.highlighted) { + text = m.highlighted[i] + } + + room := max(10, width-digits-3) + fmt.Fprintf(b, "%s %s\n", gutter, truncate(expandTabs(text), room)) +} + +func (m *FileView) Title() string { + return fmt.Sprintf("%s · !%d · %s", m.path, m.pr.ID, m.pr.Repo) +} + +func (m *FileView) Keys() help.KeyMap { + own := []key.Binding{keyFileDiff, keyLineUp, keyLineDown, keyTop, keyBottom} + return keyMap{ + short: []key.Binding{keyFileDiff, keyBack, keyHelp}, + groups: [][]key.Binding{own, {keyCopyID, keyOpen}, navBindings()}, + } +} + +// Status names the line the cursor is on, which is the one a review comment +// would be written against. It is the only thing on screen that says so. +func (m *FileView) Status() (string, bool) { + if m.status != "" { + return m.status, m.failed + } + if n, ok := m.anchorLine(); ok { + return chromeStyle.Render(fmt.Sprintf("line %d", n)), false + } + return chromeStyle.Render("removed line"), false +} + +// Prompting is false: this view has no modal state of its own yet. +func (m *FileView) Prompting() bool { return false } diff --git a/internal/ui/fileview_test.go b/internal/ui/fileview_test.go new file mode 100644 index 0000000..5a48000 --- /dev/null +++ b/internal/ui/fileview_test.go @@ -0,0 +1,255 @@ +package ui + +import ( + "strings" + "testing" + + "github.com/JacobAtchley/boardwalk/internal/azdo" + "github.com/aymanbagabas/go-udiff" + tea "github.com/charmbracelet/bubbletea" +) + +// sampleFile is a short file with one hunk in the middle: a removal and two +// additions, with untouched lines either side. Enough to tell the diff +// decoration apart from the file it decorates. +func sampleFile() (string, []*udiff.Hunk) { + text := "package tail\n\nfunc tail() {\n\tdefer close(ch)\n\tgo poll(ctx)\n}\n" + hunks := []*udiff.Hunk{hunk(3, 3, + line(udiff.Equal, "func tail() {\n"), + line(udiff.Delete, "\tgo poll()\n"), + line(udiff.Insert, "\tdefer close(ch)\n"), + line(udiff.Insert, "\tgo poll(ctx)\n"), + line(udiff.Equal, "}\n"), + )} + return text, hunks +} + +func newFileView(t *testing.T, threads []azdo.Thread) *FileView { + t.Helper() + c, pr := prDetailFixture() + text, hunks := sampleFile() + m := NewFileView(c, pr, "/internal/ui/tail.go", text, hunks, threads) + m.Body(100, 30) + return m +} + +func pressFile(t *testing.T, m *FileView, msg tea.KeyMsg) (*FileView, tea.Cmd) { + t.Helper() + updated, cmd := m.Update(msg) + next, ok := updated.(*FileView) + if !ok { + t.Fatalf("Update returned %T, want *FileView", updated) + } + return next, cmd +} + +func TestFileViewShowsTheWholeFileNotJustTheHunk(t *testing.T) { + m := newFileView(t, nil) + view := m.Body(100, 30) + + // The hunk covers lines 3 to 6. The point of this view is the rest. + for _, want := range []string{"package tail", "func tail() {", "go poll(ctx)"} { + if !strings.Contains(view, want) { + t.Errorf("the file is missing %q:\n%s", want, view) + } + } +} + +func TestFileViewMarksChangedLinesInTheGutter(t *testing.T) { + m := newFileView(t, nil) + view := m.Body(100, 30) + + rowFor := func(text string) string { + for _, l := range strings.Split(view, "\n") { + if strings.Contains(l, text) { + return l + } + } + t.Fatalf("no row holding %q:\n%s", text, view) + return "" + } + + if got := rowFor("defer close(ch)"); !strings.Contains(got, "+") { + t.Errorf("an added line is not marked: %q", got) + } + if got := rowFor("go poll()"); !strings.Contains(got, "-") { + t.Errorf("a removed line is not marked: %q", got) + } + if got := rowFor("package tail"); strings.Contains(got, "+") || strings.Contains(got, "-") { + t.Errorf("an untouched line is marked as changed: %q", got) + } +} + +func TestFileViewNumbersItsLines(t *testing.T) { + m := newFileView(t, nil) + view := m.Body(100, 30) + + for _, want := range []string{"1", "6"} { + if !strings.Contains(view, want) { + t.Errorf("line number %s is missing:\n%s", want, view) + } + } +} + +// TestFileViewTogglesTheDiffOff — d leaves the file and its highlighting and +// takes away the decoration, which is what reading it as code rather than as +// a change means. +func TestFileViewTogglesTheDiffOff(t *testing.T) { + m := newFileView(t, nil) + + m, _ = pressFile(t, m, runes("d")) + view := m.Body(100, 30) + + if strings.Contains(view, "go poll()") { + t.Errorf("a removed line is still shown with the diff off:\n%s", view) + } + if !strings.Contains(view, "go poll(ctx)") { + t.Errorf("the file itself went with the decoration:\n%s", view) + } + + m, _ = pressFile(t, m, runes("d")) + if view := m.Body(100, 30); !strings.Contains(view, "go poll()") { + t.Errorf("a second d did not bring the diff back:\n%s", view) + } +} + +func TestFileViewCursorMovesAndStops(t *testing.T) { + m := newFileView(t, nil) + first := m.cursor + + m, _ = pressFile(t, m, runes("j")) + if m.cursor != first+1 { + t.Errorf("cursor = %d after j, want %d", m.cursor, first+1) + } + + m, _ = pressFile(t, m, runes("k")) + m, _ = pressFile(t, m, runes("k")) + if m.cursor != 0 { + t.Errorf("cursor = %d at the top, want it to stop at 0 rather than wrap", m.cursor) + } + + for range 50 { + m, _ = pressFile(t, m, runes("j")) + } + if m.cursor != len(m.lines)-1 { + t.Errorf("cursor = %d at the bottom, want the last line %d", m.cursor, len(m.lines)-1) + } +} + +// TestFileViewCursorReportsTheLineAComment WouldAnchorTo — the number the +// next feature posts against, so it has to be the new file's, and a removed +// line has none. +func TestFileViewCursorReportsTheLineACommentWouldAnchorTo(t *testing.T) { + m := newFileView(t, nil) + + // Walk to the removed line, which is the fourth row: two untouched, the + // hunk's first context line, then the removal. + for range 3 { + m, _ = pressFile(t, m, runes("j")) + } + row := m.lines[m.cursor] + if row.Kind != udiff.Delete { + t.Fatalf("expected the removed row here, got %+v", row) + } + if _, ok := m.anchorLine(); ok { + t.Error("a removed line offered a new-file line to anchor a comment to") + } + + m, _ = pressFile(t, m, runes("j")) + n, ok := m.anchorLine() + if !ok { + t.Fatal("an added line offered nothing to anchor to") + } + if n != 4 { + t.Errorf("anchor line = %d, want 4 — the added line's number in the new file", n) + } +} + +func TestFileViewShowsThreadsAgainstTheirLine(t *testing.T) { + threads := []azdo.Thread{ + {ID: 1, Status: "active", File: "/internal/ui/tail.go", Line: 4, RightSide: true, + Comments: []azdo.ThreadComment{{ID: 1, Author: "Dev", Text: "this still races"}}}, + {ID: 2, Status: "active", File: "/other.go", Line: 4, RightSide: true, + Comments: []azdo.ThreadComment{{ID: 1, Author: "Dev", Text: "another file"}}}, + } + m := newFileView(t, threads) + view := m.Body(100, 30) + + if !strings.Contains(view, "this still races") { + t.Errorf("the thread on this file is missing:\n%s", view) + } + if strings.Contains(view, "another file") { + t.Errorf("another file's thread is in this one's view:\n%s", view) + } +} + +func TestFileViewEscapePops(t *testing.T) { + m := newFileView(t, nil) + + _, cmd := pressFile(t, m, tea.KeyMsg{Type: tea.KeyEsc}) + if cmd == nil { + t.Fatal("esc did nothing") + } + if _, ok := cmd().(PopMsg); !ok { + t.Errorf("esc sent %T, want PopMsg", cmd()) + } +} + +func TestFileViewTitleNamesTheFileAndPullRequest(t *testing.T) { + m := newFileView(t, nil) + title := m.Title() + + if !strings.Contains(title, "tail.go") { + t.Errorf("title = %q, want the file in it", title) + } + if !strings.Contains(title, "512") { + t.Errorf("title = %q, want the pull request in it", title) + } +} + +// TestFileViewOfAFileWithNoHunks — a file listed as changed whose text is the +// same at both commits. It is still worth opening; it just has nothing marked. +func TestFileViewOfAFileWithNoHunks(t *testing.T) { + c, pr := prDetailFixture() + m := NewFileView(c, pr, "/x.go", "one\ntwo\n", nil, nil) + + view := m.Body(100, 30) + if !strings.Contains(view, "one") || !strings.Contains(view, "two") { + t.Errorf("the file is not shown:\n%s", view) + } +} + +// TestFileViewEnterIsWiredFromTheDiffPane — the view is unreachable +// otherwise, and a feature nobody can open is not a feature. +func TestFileViewEnterIsWiredFromTheDiffPane(t *testing.T) { + r := newPRDiff(t, azdo.Change{Path: "/internal/ui/logs.go", ChangeType: "edit"}) + r.send(fileDiffMsg{PR: 512, Path: "/internal/ui/logs.go", + Diff: fileDiff{text: "one\ntwo\n", hunks: nil}}) + + cmd := r.send(tea.KeyMsg{Type: tea.KeyEnter}) + if cmd == nil { + t.Fatal("enter on a changed file did nothing") + } + push, ok := cmd().(PushMsg) + if !ok { + t.Fatalf("enter sent %T, want PushMsg", cmd()) + } + if _, ok := push.View.(*FileView); !ok { + t.Errorf("enter pushed %T, want the file view", push.View) + } +} + +// TestFileViewIsNotOpenedBeforeItsContentArrives — enter on a row whose fetch +// has not landed would push an empty pane over the list with nothing to say +// why. +func TestFileViewIsNotOpenedBeforeItsContentArrives(t *testing.T) { + r := newPRDiff(t, azdo.Change{Path: "/internal/ui/logs.go", ChangeType: "edit"}) + + cmd := r.send(tea.KeyMsg{Type: tea.KeyEnter}) + if cmd == nil { + return // nothing pushed, which is the point + } + if _, ok := cmd().(PushMsg); ok { + t.Error("enter opened a file whose content had not arrived") + } +} diff --git a/internal/ui/highlight.go b/internal/ui/highlight.go new file mode 100644 index 0000000..37012b7 --- /dev/null +++ b/internal/ui/highlight.go @@ -0,0 +1,89 @@ +package ui + +import ( + "os" + "strings" + + "github.com/alecthomas/chroma/v2/formatters" + "github.com/alecthomas/chroma/v2/lexers" + "github.com/alecthomas/chroma/v2/styles" + "github.com/charmbracelet/lipgloss" + "golang.org/x/term" +) + +// Syntax highlighting for the file view. +// +// The diff pane colours whole lines by what the change did to them, which is +// the right answer there — three lines of context either side is not enough +// code to be worth reading as code. A whole file is, so it is coloured as +// source and the change is marked in the gutter instead. + +// highlightLines styles a file and returns it one line per entry. +// +// Per line rather than as one string because the file view interleaves: a +// removed line goes between two of these, and a review comment goes under +// one. Chroma re-emits its style at the start of every line and closes it +// with a reset, even in the middle of a token spanning several — so splitting +// its output is safe, and a block comment's third line keeps its colour. +// +// A file chroma has no lexer for comes back as itself. Guessing at a language +// makes a mess of one that is not it, and plain text is what a reviewer had +// before this existed. +func highlightLines(text, path string) []string { + lines := splitLines(text) + if len(lines) == 0 { + return nil + } + + style := highlightStyle(term.IsTerminal(int(os.Stdout.Fd())), lipgloss.HasDarkBackground()) + if style == "" { + return lines + } + + lexer := lexers.Match(path) + if lexer == nil { + return lines + } + + it, err := lexer.Tokenise(nil, text) + if err != nil { + return lines + } + + var out strings.Builder + if err := formatters.Get("terminal256").Format(&out, styles.Get(style), it); err != nil { + return lines + } + + got := splitLines(out.String()) + // A lexer that somehow changed the line count would put every marker and + // every comment against the wrong line, which reads as a confidently + // wrong pane rather than as a broken one. The unstyled text is the + // better failure. + if len(got) != len(lines) { + return lines + } + return got +} + +// highlightStyle names the chroma style to use, or an empty string for no +// highlighting at all. +// +// The light-versus-dark answer comes from lipgloss, for the reason +// markdownStyle's does: asking the terminal directly means an OSC query from +// under Bubble Tea, which blocks until termenv's timeout. Bubble Tea warms +// lipgloss's copy before any program can take the terminal, and lipgloss +// caches it. +// +// Not a terminal means no styling, which matches what glamour does with the +// same question and keeps a redirected file, and a test run, as plain text. +func highlightStyle(tty, dark bool) string { + switch { + case !tty: + return "" + case dark: + return "monokai" + default: + return "github" + } +} diff --git a/internal/ui/highlight_test.go b/internal/ui/highlight_test.go new file mode 100644 index 0000000..7dea950 --- /dev/null +++ b/internal/ui/highlight_test.go @@ -0,0 +1,126 @@ +package ui + +import ( + "os" + "strings" + "testing" +) + +func TestHighlightKeepsOneEntryPerLine(t *testing.T) { + // Interleaving diff markers and review comments means indexing the + // highlighted output by line, so it has to come back with exactly as many + // lines as it went in with — no more, no fewer, whatever the lexer made + // of the source. + src := "package x\n\n/* a comment\n over three\n lines */\nfunc f() {}\n" + got := highlightLines(src, "x.go") + + if want := len(splitLines(src)); len(got) != want { + t.Fatalf("highlighted %d lines, want %d:\n%q", len(got), want, got) + } + if !strings.Contains(got[5], "func f() {}") { + t.Errorf("line 6 = %q, want the source text in it", got[5]) + } +} + +func TestHighlightLeavesTheTextIntact(t *testing.T) { + // Whatever styling is or is not applied, the code itself has to survive: + // a pane that silently drops a character is worse than one with no + // colour at all. + src := "func f(a int) (int, error) {\n\treturn a * 2, nil\n}\n" + for i, l := range highlightLines(src, "x.go") { + want := splitLines(src)[i] + if stripANSI(l) != want { + t.Errorf("line %d = %q, want %q once the styling is removed", i, stripANSI(l), want) + } + } +} + +// TestHighlightOfAnUnknownExtension — a file chroma has no lexer for must +// come back as itself rather than being guessed at or dropped. +func TestHighlightOfAnUnknownExtension(t *testing.T) { + src := "some notes\nand more\n" + got := highlightLines(src, "notes.unknownext") + + if len(got) != 2 { + t.Fatalf("got %d lines, want 2: %q", len(got), got) + } + for i, l := range got { + if l != splitLines(src)[i] { + t.Errorf("line %d = %q, want it untouched", i, l) + } + } +} + +func TestHighlightOfAnEmptyFile(t *testing.T) { + if got := highlightLines("", "x.go"); len(got) != 0 { + t.Errorf("got %d lines for an empty file, want none: %q", len(got), got) + } +} + +// TestHighlightStylePicksForTheTerminal mirrors markdownStyle: the light and +// dark answers come from the value Bubble Tea warms before the program takes +// the terminal, and not being a terminal means no styling at all — which is +// also why the tests above can compare against the raw source. +func TestHighlightStylePicksForTheTerminal(t *testing.T) { + if got := highlightStyle(false, true); got != "" { + t.Errorf("highlightStyle(notty) = %q, want no style at all", got) + } + dark, light := highlightStyle(true, true), highlightStyle(true, false) + if dark == "" || light == "" { + t.Fatalf("a terminal got no style: dark=%q light=%q", dark, light) + } + if dark == light { + t.Errorf("light and dark terminals share the style %q", dark) + } +} + +// TestHighlightNeverQueriesTheTerminal guards the same trap the glamour fix +// closed: a styling library that asks the terminal what colour it is, from +// under Bubble Tea, blocks until termenv gives up. chroma is handed the +// answer instead of being left to find it. +func TestHighlightNeverQueriesTheTerminal(t *testing.T) { + entries, err := os.ReadDir(".") + if err != nil { + t.Fatalf("could not read the package directory: %v", err) + } + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + src, err := os.ReadFile(name) + if err != nil { + t.Fatalf("could not read %s: %v", name, err) + } + for i, line := range strings.Split(string(src), "\n") { + // Comments are where the trap is explained, and explaining it is + // the opposite of falling into it. + if strings.HasPrefix(strings.TrimSpace(line), "//") { + continue + } + if strings.Contains(line, "termenv.") || strings.Contains(line, "WithAutoStyle") { + t.Errorf("%s:%d asks the terminal about itself; take the answer from "+ + "lipgloss instead, which Bubble Tea warms before the program starts", + name, i+1) + } + } + } +} + +// stripANSI removes the escape sequences a styled line carries, so a test can +// compare the text underneath. +func stripANSI(s string) string { + var b strings.Builder + for i := 0; i < len(s); { + if s[i] == 0x1b { + for i < len(s) && s[i] != 'm' { + i++ + } + i++ // the m itself + continue + } + b.WriteByte(s[i]) + i++ + } + return b.String() +} diff --git a/internal/ui/keys.go b/internal/ui/keys.go index a0da427..717a422 100644 --- a/internal/ui/keys.go +++ b/internal/ui/keys.go @@ -79,6 +79,23 @@ var ( keyQueue = key.NewBinding(key.WithKeys("N"), key.WithHelp("N", "new run")) keyCancelBuild = key.NewBinding(key.WithKeys("C"), key.WithHelp("C", "cancel run")) + // keyFileDiff toggles the change marks in the file view off, leaving the + // file and its highlighting. "d" is the drafts filter on the pull request + // list, which is two views further up — keyDiff's objection is to a key + // meaning one thing on a list and another on the pane that list opens, + // and this is not that pane. + // keyFileOpen opens the selected file whole, from the diff pane's list. + // enter is free there and means "open this" on every other list in the + // program. + keyFileOpen = key.NewBinding(key.WithKeys("enter"), key.WithHelp("enter", "open the file")) + keyFileDiff = key.NewBinding(key.WithKeys("d"), key.WithHelp("d", "diff marks")) + // keyLineUp and keyLineDown move the file view's line cursor. The list + // views read the same keys straight off msg.String(); this one is + // matched through a binding because the cursor it moves is what a review + // comment anchors to, and the help panel has to name it. + keyLineUp = key.NewBinding(key.WithKeys("up", "k"), key.WithHelp("↑/k", "line up")) + keyLineDown = key.NewBinding(key.WithKeys("down", "j"), key.WithHelp("↓/j", "line down")) + keyApprove = key.NewBinding(key.WithKeys("A"), key.WithHelp("A", "approve")) keyWait = key.NewBinding(key.WithKeys("W"), key.WithHelp("W", "wait for author")) keyReject = key.NewBinding(key.WithKeys("X"), key.WithHelp("X", "reject")) diff --git a/internal/ui/prdiff.go b/internal/ui/prdiff.go index 61cb73e..4c7805d 100644 --- a/internal/ui/prdiff.go +++ b/internal/ui/prdiff.go @@ -150,6 +150,11 @@ func hasNewSide(changeType string) bool { return !isChange(changeType, "delete") // the one line explaining why there are none. type fileDiff struct { hunks []*udiff.Hunk + // text is the side of the file the file view shows: the new one, or the + // old one for a file the pull request deleted. It is kept rather than + // discarded because buildFileDiff has already read it to produce the + // hunks, so the file view costs no request of its own. + text string // note says why a file is shown as a sentence rather than as a diff — // binary, or past the size cap. note string @@ -345,6 +350,13 @@ func buildFileDiff(c *azdo.Client, repoID, path, changeType, oldSHA, newSHA stri return fileDiff{note: note} } + // The new side, or the old one when there is no new side — a deleted + // file is still worth opening, and every line of it reads as removed. + text := after + if text == "" { + text = before + } + // Lines rather than Strings: Strings diffs runes and only then widens the // edits to line boundaries, which reports a line whose neighbour changed as // removed and re-added. On a review diff that is noise — the reader has to @@ -355,7 +367,7 @@ func buildFileDiff(c *azdo.Client, repoID, path, changeType, oldSHA, newSHA stri if err != nil { return fileDiff{err: fmt.Sprintf("could not diff the file: %v", err)} } - return fileDiff{hunks: unified.Hunks} + return fileDiff{hunks: unified.Hunks, text: text} } // unrenderable reports why a file will not be shown as a diff, or "" when it @@ -467,6 +479,30 @@ func (m *PullRequestDiff) Update(msg tea.Msg) (View, tea.Cmd) { } } + if key.Matches(msg, keyFileOpen) { + path, ok := m.selectedPath() + if !ok { + return m, nil + } + d, known := m.diffs[path] + switch { + case !known: + // Pushing now would put an empty pane over the list with + // nothing on it to say the content is still coming. + m.status, m.failed = "still reading the file…", false + return m, nil + case d.err != "": + m.status, m.failed = d.err, true + return m, nil + case d.note != "": + // Binary, or past the size cap. The note already says which. + m.status, m.failed = d.note, false + return m, nil + } + file := NewFileView(m.client, m.pr, path, d.text, d.hunks, m.threads) + return m, func() tea.Msg { return PushMsg{View: file} } + } + if key.Matches(msg, keyRefresh) { // Emptied in place rather than replaced: the rows already on screen // hold a reference to this map, and handing the view a new one