From a7560776dd25305a487225f4854e407273089bad Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Sun, 11 Oct 2026 01:35:13 -0700 Subject: [PATCH] Show diff load progress and stop delta on leave MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A large diff spent about 2s in delta behind a still "loading diff…" line, which looked like a frozen screen. The read and the layout are now two steps, so the loading line shows a spinner, the step, the line count, and the time so far. Leaving the diff, or starting another layout, now kills the running delta. Before, its answer was dropped but it ran to the end on one CPU core. Co-Authored-By: Claude Opus 5.5 --- README.md | 5 ++ docs/improvements.md | 6 ++ features_test.go | 20 ++++++- git.go | 9 ++- main_test.go | 68 +++++++++++++++++++++-- model.go | 128 +++++++++++++++++++++++++++++++------------ view.go | 13 ++++- 7 files changed, 202 insertions(+), 47 deletions(-) diff --git a/README.md b/README.md index b5312e2..c9591f2 100644 --- a/README.md +++ b/README.md @@ -119,6 +119,11 @@ for removals, magenta hunk headers. The diff is computed against the repository' (`git diff ...`), so it shows only the changes introduced on that branch since it diverged. The view is scrollable for large diffs; the header shows which base it was compared to. +A large diff can take a few seconds to lay out through delta (about 2s for 120,000 lines). While +it loads, the view shows a spinner, the step, the line count and the time so far, for example +`⠹ delta is styling 121,800 lines… 1.4s`. The keys keep working: `q` goes back at once and stops +delta, so a diff you no longer want does not keep a CPU core busy. + ## Resolving the default branch The default branch is used as the diff base, as the merge target for the `✓` indicator, and to diff --git a/docs/improvements.md b/docs/improvements.md index da7d4e8..b11207f 100644 --- a/docs/improvements.md +++ b/docs/improvements.md @@ -248,6 +248,12 @@ It left two findings on purpose: dropping the test-only `riskCommitCount`/`loadD (churn in a dozen tests for no behavior change), and keying the merge sets by full ref (it only matters for a local branch literally named like a remote one, e.g. `origin/x`). +**`v` progress and delta cancel (2026-10-11).** A 121,800-line diff spent 0.05s in `git diff` and +1.9s in delta behind a still "loading diff…" line, which read as a frozen screen. The read and the +layout are now two cmds (`diffReadMsg`, then `styleCmd`), so the loading line turns a spinner and +gives the step, the line count and the time. Leaving the diff, or starting another layout, kills +the running delta through `diffCancel`; before, its answer was dropped but it ran to the end. + **12. `main.go` split** into `git.go`, `model.go`, `view.go`, `cli.go`, `settings.go`, `proc_*.go` by an AST script — no hand edits in the move. **13.** `BINDIR` was already overridable; the Makefile now rebuilds on any `*.go` change. diff --git a/features_test.go b/features_test.go index 65b8772..8818a99 100644 --- a/features_test.go +++ b/features_test.go @@ -356,6 +356,22 @@ func TestDiffLoadsInBackground(t *testing.T) { if o := stripANSI(m.diffView()); !strings.Contains(o, "loading diff") { t.Fatalf("the view must say it is loading:\n%s", o) } + // The batch is the read and the spinner tick, in that order; the tick is + // not run, as waiting on its timer only slows the test. + read := cmd().(tea.BatchMsg)[0]().(diffReadMsg) + stale := read + stale.seq-- + if nm, c := m.Update(stale); c != nil || nm.(model).diffRaw != "" { + t.Fatal("a stale read must be dropped, and must not start a layout") + } + nm, cmd = m.Update(read) + m = nm.(model) + if cmd == nil || m.diffBase != "origin/main" || !m.diffLoading { + t.Fatalf("the read must land and start the layout: base %q", m.diffBase) + } + if o := stripANSI(m.diffView()); !strings.Contains(o, "styling 8 lines…") || strings.Contains(o, "delta is") { + t.Fatalf("the view must give the size, and not name a delta that is not there:\n%s", o) + } msg := cmd().(diffLoadedMsg) old := msg old.seq-- @@ -366,8 +382,8 @@ func TestDiffLoadsInBackground(t *testing.T) { } nm, _ = m.Update(msg) m = nm.(model) - if m.diffLoading || !strings.Contains(strings.Join(m.diffLines, "\n"), "+b") || m.diffBase != "origin/main" { - t.Fatalf("diff not applied: base %q lines %q", m.diffBase, m.diffLines) + if m.diffLoading || !strings.Contains(strings.Join(m.diffLines, "\n"), "+b") { + t.Fatalf("diff not applied: lines %q", m.diffLines) } } diff --git a/git.go b/git.go index 953b56a..6750184 100644 --- a/git.go +++ b/git.go @@ -835,17 +835,20 @@ func loadDiffRef(ref, name string) (diff, base string, err error) { // its output is used as-is, so side-by-side, line numbers and theme all come // from the user's own delta config; the width is passed so delta lays the // columns out for this terminal. Without delta (or if it fails) the raw lines -// are returned for colorizeDiffLine. -func styleDiff(diff string, width int) (lines []string, styled bool) { +// are returned for colorizeDiffLine. Cancelling ctx kills delta. +func styleDiff(ctx context.Context, diff string, width int) (lines []string, styled bool) { if strings.TrimSpace(diff) == "" { return nil, false } if path, err := exec.LookPath("delta"); err == nil { - cmd := exec.Command(path, "--paging=never", "--width="+strconv.Itoa(width)) + cmd := exec.CommandContext(ctx, path, "--paging=never", "--width="+strconv.Itoa(width)) cmd.Stdin = strings.NewReader(diff) if out, err := cmd.Output(); err == nil { return strings.Split(strings.TrimRight(string(out), "\n"), "\n"), true } + if ctx.Err() != nil { + return nil, false // nobody wants the answer; skip the fallback's work too + } } // The file contents are someone else's text; clean each line like a subject. // Tabs are widened first, or cleanText would flatten indentation to one space. diff --git a/main_test.go b/main_test.go index a7e5496..4c0204d 100644 --- a/main_test.go +++ b/main_test.go @@ -1,6 +1,7 @@ package main import ( + "context" "fmt" "os" "os/exec" @@ -2769,11 +2770,11 @@ func TestStyleDiffUsesDeltaWhenPresent(t *testing.T) { bin := t.TempDir() t.Setenv("PATH", bin) - lines, styled := styleDiff(diff, 80) + lines, styled := styleDiff(context.Background(), diff, 80) if styled || len(lines) != 6 { t.Fatalf("without delta the raw lines must come back: styled=%v lines=%q", styled, lines) } - if lines, styled := styleDiff(" \n", 80); styled || lines != nil { + if lines, styled := styleDiff(context.Background(), " \n", 80); styled || lines != nil { t.Fatalf("an empty diff has no lines: styled=%v lines=%q", styled, lines) } @@ -2781,7 +2782,7 @@ func TestStyleDiffUsesDeltaWhenPresent(t *testing.T) { if err := os.WriteFile(fake, []byte("#!/bin/sh\necho \"delta $*\"\n/bin/cat >/dev/null\n"), 0o755); err != nil { t.Fatal(err) } - lines, styled = styleDiff(diff, 120) + lines, styled = styleDiff(context.Background(), diff, 120) if !styled || len(lines) != 1 || lines[0] != "delta --paging=never --width=120" { t.Fatalf("delta must run with the terminal width and its output shown as-is: styled=%v lines=%q", styled, lines) } @@ -2789,7 +2790,7 @@ func TestStyleDiffUsesDeltaWhenPresent(t *testing.T) { if err := os.WriteFile(fake, []byte("#!/bin/sh\nexit 1\n"), 0o755); err != nil { t.Fatal(err) } - if lines, styled := styleDiff(diff, 80); styled || len(lines) != 6 { + if lines, styled := styleDiff(context.Background(), diff, 80); styled || len(lines) != 6 { t.Fatalf("a failing delta must fall back to the raw lines: styled=%v lines=%q", styled, lines) } } @@ -2803,7 +2804,7 @@ func TestDiffRerendersOnResize(t *testing.T) { t.Fatal(err) } m := model{width: 80, height: 24, state: stateDiff, diffRaw: "-a\n+b\n"} - m.diffLines, m.diffStyled = styleDiff(m.diffRaw, m.width) + m.diffLines, m.diffStyled = styleDiff(context.Background(), m.diffRaw, m.width) nm, cmd := m.Update(tea.WindowSizeMsg{Width: 140, Height: 40}) m = nm.(model) if cmd == nil { @@ -2828,6 +2829,63 @@ func TestDiffRerendersOnResize(t *testing.T) { } } +// Leaving the diff kills a delta still at work: its answer would be dropped, +// and on a big diff it would hold a CPU core for seconds. +func TestLeavingDiffStopsDelta(t *testing.T) { + bin := t.TempDir() + t.Setenv("PATH", bin) + // exec, so the kill reaches the process holding delta's output pipe. + if err := os.WriteFile(filepath.Join(bin, "delta"), []byte("#!/bin/sh\nexec /bin/sleep 30\n"), 0o755); err != nil { + t.Fatal(err) + } + m := model{width: 80, height: 24, state: stateDiff, diffRaw: "-a\n+b\n", diffLoading: true} + cmd := m.styleCmd() + done := make(chan tea.Msg, 1) + go func() { done <- cmd() }() + nm, _ := m.Update(key("q")) + if nm.(model).state != stateList { + t.Fatal("q must leave the diff") + } + select { + case msg := <-done: + if lm := msg.(diffLoadedMsg); lm.lines != nil { + t.Fatalf("a stopped run must skip the raw-line fallback, got %q", lm.lines) + } + case <-time.After(10 * time.Second): + t.Fatal("delta still runs after the user left the diff") + } +} + +// The loading line turns while the diff loads, and only one tick chain runs: +// a tick for an older request, or after the load lands, does not re-arm. +func TestDiffSpinnerTicksWhileLoading(t *testing.T) { + m := model{width: 80, height: 24, state: stateDiff, diffSeq: 2, diffLoading: true, + diffNote: "delta is styling 121,800 lines…", diffStart: time.Now().Add(-1500 * time.Millisecond)} + nm, cmd := m.Update(diffTickMsg{seq: 2}) + m = nm.(model) + if m.spinnerFrame != 1 || cmd == nil { + t.Fatalf("the tick must advance and re-arm: frame %d", m.spinnerFrame) + } + if o := stripANSI(m.diffView()); !strings.Contains(o, spinnerFrames[1]+" delta is styling 121,800 lines… 1.5s") { + t.Fatalf("the loading line must show the spinner, the step, and the time:\n%s", o) + } + if _, cmd := m.Update(diffTickMsg{seq: 1}); cmd != nil { + t.Fatal("a tick for an older request must not re-arm") + } + m.diffLoading = false + if _, cmd := m.Update(diffTickMsg{seq: 2}); cmd != nil { + t.Fatal("the tick must not re-arm once the diff has landed") + } +} + +func TestGroupDigits(t *testing.T) { + for n, want := range map[int]string{0: "0", 999: "999", 1000: "1,000", 121800: "121,800", 1234567: "1,234,567"} { + if got := groupDigits(n); got != want { + t.Errorf("groupDigits(%d) = %q, want %q", n, got, want) + } + } +} + // result returns m's deletion result for the named branch, failing if none ran. func result(t *testing.T, m model, name string) deleteResult { t.Helper() diff --git a/model.go b/model.go index 4f07324..fd25bc4 100644 --- a/model.go +++ b/model.go @@ -1,7 +1,9 @@ package main import ( + "context" "fmt" + "os/exec" "path" "slices" "sort" @@ -124,16 +126,19 @@ type model struct { // confirmAfterRisk). The confirm screen opens when the counts land. measuring bool - spinnerFrame int // animation frame for the deleting spinner (deletion counts derive from results) + spinnerFrame int // animation frame for the deleting and diff-loading spinners (deletion counts derive from results) - diffBranch string // branch whose diff is shown in stateDiff - diffBase string // base ref the diff was computed against - diffRaw string // unified diff as git produced it; delta is re-run from it on resize - diffLines []string // display lines of the diff being viewed - diffStyled bool // lines came from delta and carry their own colors - diffTop int // scroll offset within diffLines - diffLoading bool // the diff (or its re-layout) is being made in the background - diffSeq int // bumped per diff request, so a slow answer for an old one is dropped + diffBranch string // branch whose diff is shown in stateDiff + diffBase string // base ref the diff was computed against + diffRaw string // unified diff as git produced it; delta is re-run from it on resize + diffLines []string // display lines of the diff being viewed + diffStyled bool // lines came from delta and carry their own colors + diffTop int // scroll offset within diffLines + diffLoading bool // the diff (or its re-layout) is being made in the background + diffSeq int // bumped per diff request, so a slow answer for an old one is dropped + diffNote string // what the loading line says the load is doing + diffStart time.Time // when v was pressed; the loading line shows the time since + diffCancel context.CancelFunc // stops the running layout (see stopStyle) bodyTop int // scroll offset within the confirm/force/result body (see page) @@ -228,39 +233,67 @@ type riskMsg struct { counts []riskCount } -// diffLoadedMsg carries a diff made in the background; seq ties it to one request. +// diffReadMsg carries the diff git produced; seq ties it to one request. The +// read and the layout are two steps so the loading line can give the size +// while delta works on it. +type diffReadMsg struct { + seq int + base string + raw string + delta bool // delta is on the PATH, so the layout step is the slow one + err error +} + +// diffCmd reads b's diff off the update loop. +func diffCmd(seq int, b branch) tea.Cmd { + return func() tea.Msg { + msg := diffReadMsg{seq: seq} + msg.raw, msg.base, msg.err = loadDiffRef(b.ref(), b.name) + _, err := exec.LookPath("delta") + msg.delta = err == nil + return msg + } +} + +// diffLoadedMsg carries a diff laid out in the background; seq ties it to one request. type diffLoadedMsg struct { seq int - base string // "" on a re-layout: the base is unchanged - raw string lines []string styled bool - err error } -// diffCmd reads b's diff and lays it out, off the update loop: a big diff -// through delta takes long enough to freeze the screen. -func diffCmd(seq int, b branch, width int) tea.Cmd { +// styleCmd lays m.diffRaw out off the update loop: a big diff through delta +// takes seconds. An earlier run still at work is stopped first. +func (m *model) styleCmd() tea.Cmd { + m.stopStyle() + ctx, cancel := context.WithCancel(context.Background()) + m.diffCancel = cancel + seq, raw, width := m.diffSeq, m.diffRaw, m.width return func() tea.Msg { msg := diffLoadedMsg{seq: seq} - msg.raw, msg.base, msg.err = loadDiffRef(b.ref(), b.name) - if msg.err == nil { - msg.lines, msg.styled = styleDiff(msg.raw, width) - } + msg.lines, msg.styled = styleDiff(ctx, raw, width) return msg } } +// stopStyle kills a running delta. Its answer would be dropped anyway, and on +// a big diff it holds a CPU core for seconds after the user has left. +func (m *model) stopStyle() { + if m.diffCancel != nil { + m.diffCancel() + m.diffCancel = nil + } +} + // restyleTickMsg fires a moment after a resize (see the WindowSizeMsg case). type restyleTickMsg struct{ seq int } -// restyleCmd re-runs delta on a diff already read, for a new terminal width. -func restyleCmd(seq int, raw string, width int) tea.Cmd { - return func() tea.Msg { - msg := diffLoadedMsg{seq: seq, raw: raw} - msg.lines, msg.styled = styleDiff(raw, width) - return msg - } +// diffTickMsg advances the loading-line spinner. seq ties it to one request, +// so a quick v, q, v cannot leave two tick chains running. +type diffTickMsg struct{ seq int } + +func diffTickCmd(seq int) tea.Cmd { + return tea.Tick(spinnerInterval, func(time.Time) tea.Msg { return diffTickMsg{seq} }) } // branchDeletedMsg reports the outcome of one branch's async deletion; idx is @@ -273,9 +306,12 @@ type branchDeletedMsg struct { // spinnerTickMsg advances the deleting-view spinner animation. type spinnerTickMsg struct{} -// spinnerFrames are the braille frames cycled while deletions are in flight. +// spinnerFrames are the braille frames cycled while deletions are in flight +// or a diff loads, one per spinnerInterval. var spinnerFrames = []string{"⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"} +const spinnerInterval = 120 * time.Millisecond + // deleteBranchCmd wraps the deleteBranch worker as a tea.Cmd so deletions run off // the update loop. It captures only a branch value (never the model), so each runs // independently and concurrently under tea.Batch. @@ -287,7 +323,7 @@ func deleteBranchCmd(idx int, b branch, force, wantRemote bool) tea.Cmd { // spinnerTickCmd schedules the next spinner frame. func spinnerTickCmd() tea.Cmd { - return tea.Tick(120*time.Millisecond, func(time.Time) tea.Msg { return spinnerTickMsg{} }) + return tea.Tick(spinnerInterval, func(time.Time) tea.Msg { return spinnerTickMsg{} }) } // loadFirst runs the startup reads. The repo check runs beside them rather than @@ -500,7 +536,7 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil // a later resize took over, or the user left } m.diffLoading = true - return m, restyleCmd(m.diffSeq, m.diffRaw, m.width) + return m, m.styleCmd() case mergeInfoMsg: if msg.gen == m.loadGen { m.applyMergeInfo(msg.info) @@ -509,23 +545,41 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil case riskMsg: return m.landRisk(msg) - case diffLoadedMsg: + case diffReadMsg: if msg.seq != m.diffSeq || m.state != stateDiff { return m, nil // the user asked for another diff, or left } - m.diffLoading = false if msg.err != nil { + m.diffLoading = false m.err = msg.err.Error() m.state = stateList return m, nil } - if msg.base != "" { - m.diffBase = msg.base + m.diffBase, m.diffRaw = msg.base, msg.raw + if strings.TrimSpace(msg.raw) == "" { + m.diffLoading = false + return m, nil + } + m.diffNote = "styling " + groupDigits(strings.Count(msg.raw, "\n")) + " lines…" + if msg.delta { + m.diffNote = "delta is " + m.diffNote } - m.diffRaw = msg.raw + return m, m.styleCmd() + case diffLoadedMsg: + if msg.seq != m.diffSeq || m.state != stateDiff { + return m, nil // the user asked for another diff, or left + } + m.diffLoading = false + m.stopStyle() // the run is over; this only frees its context m.diffLines, m.diffStyled = msg.lines, msg.styled m.clampDiff() return m, nil + case diffTickMsg: + if msg.seq == m.diffSeq && m.state == stateDiff && m.diffLoading { + m.spinnerFrame++ + return m, diffTickCmd(msg.seq) + } + return m, nil case undoDoneMsg: return m, m.applyUndo(msg) case repoLoadedMsg: @@ -804,8 +858,9 @@ func (m model) updateList(msg tea.KeyMsg) (tea.Model, tea.Cmd) { m.diffBranch, m.diffBase = b.name, "" m.diffRaw, m.diffLines, m.diffStyled = "", nil, false m.diffTop, m.diffLoading = 0, true + m.diffNote, m.diffStart = "loading diff…", time.Now() m.state = stateDiff - return m, diffCmd(m.diffSeq, *b, m.width) + return m, tea.Batch(diffCmd(m.diffSeq, *b), diffTickCmd(m.diffSeq)) } case "c": if b := m.cur(); b != nil && !b.isCurrent && !m.switching && !m.fetching { @@ -948,6 +1003,7 @@ func (m *model) clampDiff() { func (m model) updateDiff(msg tea.KeyMsg) (tea.Model, tea.Cmd) { switch msg.String() { case "q", "esc", "v": + m.stopStyle() m.state = stateList case "ctrl+c": return m, tea.Quit diff --git a/view.go b/view.go index 032bb6c..96d1f5e 100644 --- a/view.go +++ b/view.go @@ -4,6 +4,7 @@ import ( "fmt" "strconv" "strings" + "time" "github.com/charmbracelet/lipgloss" "github.com/charmbracelet/x/ansi" @@ -180,7 +181,8 @@ func (m model) diffView() string { if len(m.diffLines) == 0 { msg := "no changes — branch matches " + base if m.diffLoading { - msg = "loading diff…" + spin := spinnerFrames[m.spinnerFrame%len(spinnerFrames)] + msg = fmt.Sprintf("%s %s %.1fs", spin, m.diffNote, time.Since(m.diffStart).Seconds()) } b.WriteString(dimStyle.Render(msg)) b.WriteString("\n\n") @@ -814,3 +816,12 @@ func (m model) resultParts() (header, body, footer []string) { } func (m model) resultView() string { return m.page(m.resultParts()) } + +// groupDigits writes n with commas between groups of three digits (121,800). +func groupDigits(n int) string { + s := strconv.Itoa(n) + for i := len(s) - 3; i > 0; i -= 3 { + s = s[:i] + "," + s[i:] + } + return s +}