From 01600042cdd8a73813263cdba71cf430503d0f53 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 9 Aug 2026 08:17:24 +0200 Subject: [PATCH 01/16] Guard taskKey with the mutex that already guards the task ID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit taskKey is written on the goroutine NewTask spawns, under taskIDMutex, but GetTaskKey read it without the lock — and the string renders in tasks_adapter.go call that from the UI thread while a previous task's goroutine may be writing. A Go string is a two-word value, so a torn read can pair one string's pointer with another's length and index out of bounds, not merely return the wrong key. Take the lock in GetTaskKey, and read the field directly at the one call site that already holds it. No test: the failure needs two goroutines to interleave inside a two-word assignment, which nothing can schedule deterministically. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/tasks/tasks.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index df7791aaf..869f03dd5 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -61,9 +61,13 @@ type ViewBufferManager struct { writer io.Writer waitingMutex deadlock.Mutex - taskIDMutex deadlock.Mutex - Log *logrus.Entry - newTaskID int + // Guards newTaskID and taskKey, which identify the most recently requested + // task. Both are written on the goroutine NewTask spawns, and taskKey is + // read from the UI thread (GetTaskKey), so neither may be touched without + // holding this. + taskIDMutex deadlock.Mutex + Log *logrus.Entry + newTaskID int // The channel by which the currently-running task is told to read more // lines (e.g. as the user scrolls). Held in an atomic because it's swapped // out as tasks come and go while ReadLines/ReadToEnd read it from the UI @@ -115,6 +119,9 @@ type LinesToRead struct { } func (self *ViewBufferManager) GetTaskKey() string { + self.taskIDMutex.Lock() + defer self.taskIDMutex.Unlock() + return self.taskKey } @@ -488,7 +495,9 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error return } - resetOrigin := self.GetTaskKey() != key && self.onNewKey != nil + // Read taskKey directly: we already hold the mutex that guards it, and + // GetTaskKey would take it again. + resetOrigin := self.taskKey != key && self.onNewKey != nil self.taskKey = key self.taskIDMutex.Unlock() From 94018de3e803d6a7098d41e51998ca7bf95a824e Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 8 Aug 2026 18:57:12 +0200 Subject: [PATCH 02/16] Route all view origin writes through SetOriginX and SetOriginY Several methods assigned v.ox and v.oy directly: SetOrigin, CopyContent, the wrap/autoscroll branches in draw, FocusPoint, and Scroll{Up,Down,Left,Right}. Funnelling them all through SetOriginX and SetOriginY gives a single place to observe (or set a breakpoint on) every change to a view's scroll position, which makes debugging scroll behaviour much easier. This means those call sites now also get the setters' `< 0` clamps, but that is behaviour-preserving in every case: each assigned value is already >= 0. calculateNewOrigin never returns a negative number; CopyContent copies origins that are themselves always >= 0; and the draw and scroll writes are all guarded (or fed only non-negative amounts) so the result can't go below zero. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/gocui/view.go | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index 787d11475..81ab0acab 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -402,7 +402,7 @@ func (v *View) FocusPoint(cx int, cy int, scrollIntoView bool) { if scrollIntoView { height := v.InnerHeight() - v.oy = calculateNewOrigin(cy, v.oy, lineCount, height) + v.SetOriginY(calculateNewOrigin(cy, v.oy, lineCount, height)) } v.cx = cx @@ -707,15 +707,8 @@ func (v *View) CursorY() int { // implement Horizontal and Vertical scrolling with just incrementing // or decrementing ox and oy. func (v *View) SetOrigin(x, y int) { - if x < 0 { - x = 0 - } - if y < 0 { - y = 0 - } - - v.ox = x - v.oy = y + v.SetOriginX(x) + v.SetOriginY(y) } func (v *View) SetOriginX(x int) { @@ -1166,8 +1159,8 @@ func (v *View) CopyContent(from *View) { // their contents. v.lines = slices.Clone(from.lines) v.viewLines = slices.Clone(from.viewLines) - v.ox = from.ox - v.oy = from.oy + v.SetOriginX(from.ox) + v.SetOriginY(from.oy) v.cx = from.cx v.cy = from.cy } @@ -1335,14 +1328,14 @@ func (v *View) draw(isWindowFocused bool) { if maxX == 0 { return } - v.ox = 0 + v.SetOriginX(0) } v.refreshViewLinesIfNeeded() visibleViewLinesHeight := v.viewLineLengthIgnoringTrailingBlankLines() if v.Autoscroll && visibleViewLinesHeight > maxY { - v.oy = visibleViewLinesHeight - maxY + v.SetOriginY(visibleViewLinesHeight - maxY) } if len(v.viewLines) == 0 { @@ -1989,7 +1982,7 @@ func (v *View) ScrollUp(amount int) { } if amount != 0 { - v.oy -= amount + v.SetOriginY(v.oy - amount) v.cy += amount v.clearHover() @@ -2001,7 +1994,7 @@ func (v *View) ScrollUp(amount int) { func (v *View) ScrollDown(amount int) { adjustedAmount := v.adjustDownwardScrollAmount(amount) if adjustedAmount > 0 { - v.oy += adjustedAmount + v.SetOriginY(v.oy + adjustedAmount) v.cy -= adjustedAmount v.clearHover() @@ -2015,7 +2008,7 @@ func (v *View) ScrollLeft(amount int) { newOx = 0 } if newOx != v.ox { - v.ox = newOx + v.SetOriginX(newOx) v.clearHover() } @@ -2023,7 +2016,7 @@ func (v *View) ScrollLeft(amount int) { // not applying any limits to this func (v *View) ScrollRight(amount int) { - v.ox += amount + v.SetOriginX(v.ox + amount) v.clearHover() } From d65ee95942d2e6d523308c0d969ab5916f8bd47a Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 8 Aug 2026 18:58:17 +0200 Subject: [PATCH 03/16] Add LAZYGIT_SLOW_RENDER debug knob for watching async render frames Re-rendering a diff into a main view is asynchronous and lazy: the read loop fills the view a screenful at a time and refreshes as it goes. When debugging scroll-restore and flicker behaviour, the individual frames go by too fast to see. Setting LAZYGIT_SLOW_RENDER= sleeps that long after each line is written, stretching the load out so the frames become visible. It has no effect when unset, so it's safe to leave in. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/tasks/tasks.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index 869f03dd5..c9b5aaa5a 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -4,7 +4,9 @@ import ( "bufio" "fmt" "io" + "os" "os/exec" + "strconv" "sync" "sync/atomic" "time" @@ -320,6 +322,17 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix // this to work out how many more lines, if any, we still need to read. linesRead := 0 + // Set LAZYGIT_SLOW_RENDER= to sleep that long after each + // line is written to the view, stretching async loads out so the frames + // of a re-render become visible. Useful for debugging scroll/flicker + // behaviour; has no effect when the variable is unset. + var slowRenderPerLine time.Duration + if v := os.Getenv("LAZYGIT_SLOW_RENDER"); v != "" { + if ms, err := strconv.Atoi(v); err == nil { + slowRenderPerLine = time.Duration(ms) * time.Millisecond + } + } + outer: for { if stopped() { @@ -373,6 +386,10 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix lineWrittenChan <- struct{}{} linesRead++ + if slowRenderPerLine > 0 { + time.Sleep(slowRenderPerLine) + } + if linesRead == linesToRead.InitialRefreshAfter { // We have read enough lines to fill the view, so do a first refresh // here to show what we have. Continue reading and refresh again at From e3fe3210808156ed8a2cdf0cd01c963ad2ecab69 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 8 Aug 2026 19:04:11 +0200 Subject: [PATCH 04/16] Move the click-path hyperlink lookup onto View Reading a view's internal buffer belongs on the view itself, next to findHyperlinkAt, rather than in the event loop; and the view is where the lock that guards that buffer can be taken. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/gocui/gui.go | 6 ++---- pkg/gocui/view.go | 10 ++++++++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index 2f22afe01..ddb356b20 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -1763,10 +1763,8 @@ func (g *Gui) onKey(ev *GocuiEvent) error { } if ev.Key.KeyName() == MouseLeft && (ev.Key.Mod()&ModMotion) == 0 && !v.Editable && g.openHyperlink != nil { - if newY >= 0 && newY <= len(v.viewLines)-1 && newX >= 0 && newX <= len(v.viewLines[newY].line)-1 { - if link := v.viewLines[newY].line[newX].hyperlink; link != "" { - return g.openHyperlink(link, v.name) - } + if link := v.hyperlinkAt(newX, newY); link != "" { + return g.openHyperlink(link, v.name) } } diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index 81ab0acab..e05446207 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -2116,6 +2116,16 @@ func (v *View) onMouseMove(x int, y int) { } } +// hyperlinkAt returns the hyperlink at the given position of the view's +// content, or an empty string if there is none. +func (v *View) hyperlinkAt(x, y int) string { + if y < 0 || y >= len(v.viewLines) || x < 0 || x >= len(v.viewLines[y].line) { + return "" + } + + return v.viewLines[y].line[x].hyperlink +} + func (v *View) findHyperlinkAt(x, y int) *SearchPosition { linkStr := v.viewLines[y].line[x].hyperlink if linkStr == "" { From 86c9e6a20a86fb9b77c817cdbe0373189582086d Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 8 Aug 2026 19:05:06 +0200 Subject: [PATCH 05/16] Lock the view while reading viewLines on the event-handling thread hyperlinkAt (the click path) and onMouseMove/findHyperlinkAt (hover) read v.viewLines without holding writeMutex, unlike every other reader. They run on the event-handling goroutine, so a re-render on the task goroutine can shrink or rebuild viewLines between the bounds check and the indexing, causing an out-of-range panic (observed: "index out of range [60] with length 0" while hovering during a diff re-render). Take writeMutex for the duration, like the other viewLines readers do, so the check and the access see the same slice. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/gocui/view.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index e05446207..66d0cde3a 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -2098,6 +2098,9 @@ func (v *View) onMouseMove(x int, y int) { return } + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + // newCx and newCy are relative to the view port, i.e. to the visible area of the view newCx := x - v.x0 - 1 newCy := y - v.y0 - 1 @@ -2119,6 +2122,9 @@ func (v *View) onMouseMove(x int, y int) { // hyperlinkAt returns the hyperlink at the given position of the view's // content, or an empty string if there is none. func (v *View) hyperlinkAt(x, y int) string { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + if y < 0 || y >= len(v.viewLines) || x < 0 || x >= len(v.viewLines[y].line) { return "" } From 9293f03c835fbb65064f8988317f8ef89c18f3f9 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 8 Aug 2026 19:07:30 +0200 Subject: [PATCH 06/16] Add a test for a read request queued while a task reaches EOF Co-Authored-By: Claude Opus 5 (1M context) --- pkg/tasks/tasks_test.go | 80 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index dec476ba4..4211dcb08 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -12,6 +12,7 @@ import ( "github.com/jesseduffield/lazygit/pkg/gocui" "github.com/jesseduffield/lazygit/pkg/utils" + "github.com/stretchr/testify/assert" ) func getCounter() (func(), func() int) { @@ -174,6 +175,85 @@ func (d *BlankLineReader) Read(p []byte) (n int, err error) { return 1, nil } +// A dummy reader that yields the given number of blank lines and then blocks +// until unblock is closed, at which point it reports EOF. This lets a test hold +// a task in its "still loading" state for as long as it needs to. +type BlockingLineReader struct { + linesToYield int + linesYielded int + reachedEnd bool + blocked chan struct{} + unblock chan struct{} +} + +func (d *BlockingLineReader) Read(p []byte) (n int, err error) { + if d.linesYielded == d.linesToYield { + if !d.reachedEnd { + d.reachedEnd = true + close(d.blocked) + } + <-d.unblock + return 0, io.EOF + } + + d.linesYielded++ + p[0] = '\n' + return 1, nil +} + +func TestNewCmdTaskQueuedReadAtEndOfInput(t *testing.T) { + writer := bytes.NewBuffer(nil) + task := gocui.NewFakeTask() + + manager := NewViewBufferManager( + utils.NewDummyLog(), + writer, + func() {}, + func() {}, + func() {}, + func() {}, + func() gocui.Task { return task }, + // no UI thread in the test; run the view mutations inline + func(f func()) error { f(); return nil }, + ) + + reader := BlockingLineReader{ + linesToYield: 5, + blocked: make(chan struct{}), + unblock: make(chan struct{}), + } + start := func() (Cmd, io.Reader) { + // not actually starting this because it's not necessary + return ExecCmd{Cmd: exec.Command("blah")}, &reader + } + + // The initial request asks for far more lines than the reader has, so the + // task reaches EOF while that request is still the one being served. + fn := manager.NewCmdTask(start, "", LinesToRead{100, -1, nil}, func() {}) + + thenCalled := false + wg := sync.WaitGroup{} + wg.Go(func() { + _ = fn(TaskOpts{Stop: make(chan struct{}), InitialContentLoaded: func() { task.Done() }}) + }) + + <-reader.blocked + manager.ReadToEnd(func() { thenCalled = true }) + // ReadToEnd queues its request from a goroutine; wait for it to land so that + // it is definitely outstanding by the time we let the task reach EOF. + for len(*manager.readLines.Load()) == 0 { + time.Sleep(time.Millisecond) + } + close(reader.unblock) + + wg.Wait() + + /* EXPECTED: + assert.True(t, thenCalled) + ACTUAL: */ + assert.False(t, thenCalled) +} + func TestNewCmdTaskRefresh(t *testing.T) { type scenario struct { name string From 85cff19a2a216339b7424932bca1fd6685126bba Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 8 Aug 2026 19:08:12 +0200 Subject: [PATCH 07/16] Fire queued ReadToEnd callbacks when the initial read reaches EOF A task's read loop processes one LinesToRead request at a time. The initial request has a large line count and no Then callback; if the content is shorter than that, the loop hits EOF on the initial request and breaks out, abandoning any further requests still sitting in the readLines channel. So a ReadToEnd call that races a still-loading-but-shorter-than-its-initial-read view has its Then silently dropped: it isn't fired immediately (the channel was non-nil at call time) and it's never dequeued. On EOF, drain the queued requests and fire their Then callbacks before breaking out, since reaching EOF trivially satisfies any "read more" request. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/tasks/tasks.go | 15 +++++++++++++++ pkg/tasks/tasks_test.go | 3 --- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index c9b5aaa5a..dec232189 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -380,6 +380,21 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix // are UI-thread-only, so run it there. _ = self.onUIThread(self.onEndOfInput) callThen() + // Any read requests that were queued while we were reading are + // now trivially satisfied, since we've read everything. Fire + // their callbacks instead of dropping them when we break out of + // the loop below (and nil out readLines). + drain: + for { + select { + case queued := <-readLines: + if queued.Then != nil { + queued.Then() + } + default: + break drain + } + } break outer } writeToView(append(line, '\n')) diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index 4211dcb08..d9ec7e4d4 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -248,10 +248,7 @@ func TestNewCmdTaskQueuedReadAtEndOfInput(t *testing.T) { wg.Wait() - /* EXPECTED: assert.True(t, thenCalled) - ACTUAL: */ - assert.False(t, thenCalled) } func TestNewCmdTaskRefresh(t *testing.T) { From fbadbbf99a7d9488efe1e47d36cf05cdcacf689b Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 8 Aug 2026 19:09:44 +0200 Subject: [PATCH 08/16] Don't scroll a view up to fill blank space while its content is loading The layout scrolls a view up if its origin is past the bottom of its content, to avoid showing blank space (e.g. after a resize). But it measures content height by the lines loaded so far, and command/pty tasks load asynchronously. So when a view is re-rendered while scrolled down, the layout would yank it to the top because only a fraction of the content has been read yet, then leave it there once loading finished. Track whether a command task is actively reading (set synchronously when the task is created, so a layout pass in between sees it; cleared at EOF, but not when stopped, since that means a newer task is taking over) and skip the scroll-up clamp for such views. onEndOfInput already re-clamps once loading completes. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/gui/layout.go | 8 +++++++- pkg/gui/pty.go | 6 ++++++ pkg/gui/tasks_adapter.go | 4 ++++ pkg/tasks/tasks.go | 25 +++++++++++++++++++++++++ 4 files changed, 42 insertions(+), 1 deletion(-) diff --git a/pkg/gui/layout.go b/pkg/gui/layout.go index bcdc0edfc..67e695f2b 100644 --- a/pkg/gui/layout.go +++ b/pkg/gui/layout.go @@ -88,7 +88,13 @@ func (gui *Gui) layout(g *gocui.Gui) error { if !view.CanScrollPastBottom { maxOriginY -= newHeight - 1 } - if oldOriginY := view.OriginY(); oldOriginY > maxOriginY { + // Don't scroll up while the view's content is still being loaded: its + // height only reflects what has been read so far, so clamping to it now + // would yank the view to the top even though more content is on the way + // (e.g. when re-rendering a diff the user was scrolled into). + manager := gui.getViewBufferManagerForView(view) + stillLoading := manager != nil && manager.IsLoading() + if oldOriginY := view.OriginY(); oldOriginY > maxOriginY && !stillLoading { view.ScrollUp(oldOriginY - maxOriginY) // the view might not have scrolled actually (if it was at the limit // already), so we need to check if it did diff --git a/pkg/gui/pty.go b/pkg/gui/pty.go index 719f5c348..1cb8d9412 100644 --- a/pkg/gui/pty.go +++ b/pkg/gui/pty.go @@ -72,6 +72,12 @@ func (gui *Gui) newPtyTask(view *gocui.View, cmd *exec.Cmd, prefix string) error cmd.Args = withPtyGitConfig(cmd.Args, runtime.GOOS) + // Mark the view as loading synchronously now, before the layout pass: the + // actual task is created in afterLayout (below), which runs after layout, so + // without this the next layout pass would clamp the scroll position to the + // not-yet-loaded content. + gui.getManager(view).StartLoading() + // Run the pty after layout so that it gets the correct size gui.afterLayout(func() error { // Need to get the width and the pager command again because the layout might have diff --git a/pkg/gui/tasks_adapter.go b/pkg/gui/tasks_adapter.go index ccc0b308c..440bc0aab 100644 --- a/pkg/gui/tasks_adapter.go +++ b/pkg/gui/tasks_adapter.go @@ -18,6 +18,10 @@ func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error ).Debug("RunCommand") manager := gui.getManager(view) + // Mark the view as loading synchronously (before the task's goroutine runs + // and before the next layout pass) so the layout doesn't clamp the scroll + // position to the not-yet-loaded content. + manager.StartLoading() // Snapshot the view width here, on the UI thread, so the task goroutine // doesn't read the view's live dimensions while it streams output. It's diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index dec232189..a2084ea36 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -78,6 +78,11 @@ type ViewBufferManager struct { taskKey string onNewKey func() + // Whether a command task is currently reading content into the view. While + // this is true the content is still growing, so callers (e.g. the layout) + // must not clamp the view's scroll position to the amount loaded so far. + loading atomic.Bool + // beforeStart is the function that is called before starting a new task beforeStart func() refreshView func() @@ -162,6 +167,21 @@ func (self *ViewBufferManager) ReadLines(totalLines int) { } } +// IsLoading reports whether a command task is currently reading content into the +// view, meaning the content is still growing. +func (self *ViewBufferManager) IsLoading() bool { + return self.loading.Load() +} + +// StartLoading marks the view as loading content. It must be called +// synchronously when a command/pty task is started, before the task's goroutine +// runs, so that a layout pass happening in between doesn't clamp the scroll +// position to the not-yet-loaded content. It is cleared when the task reaches +// the end of its input. +func (self *ViewBufferManager) StartLoading() { + self.loading.Store(true) +} + func (self *ViewBufferManager) ReadToEnd(then func()) { if ch := self.readLines.Load(); ch != nil { readLines := *ch @@ -379,6 +399,11 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix // whether to scroll) and sets the origin, both of which // are UI-thread-only, so run it there. _ = self.onUIThread(self.onEndOfInput) + // The content is fully loaded now, so it's safe again for the + // layout to clamp the scroll position to it. We deliberately + // don't clear this when stopped (rather than EOF'd), because that + // means a newer task is taking over and is still loading. + self.loading.Store(false) callThen() // Any read requests that were queued while we were reading are // now trivially satisfied, since we've read everything. Fire From 114d18dc04a860ef38a70e0a58e4d7a96126b8fc Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 8 Aug 2026 19:10:12 +0200 Subject: [PATCH 09/16] Reset other main views' scroll after copying content, not before MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refreshMainViews reset the scroll position of every other main view at the very top, before moveMainContextPairToTop runs its CopyContent. CopyContent copies the previously-shown view's content into the now-visible one to avoid a blank frame during the async re-render — but because the reset ran first, it had already zeroed the origin of that soon-to-be-copied source view. The placeholder therefore always appeared scrolled to the top, jumping away from wherever the screen actually was, on every cross-pair transition. Move the reset to after the copy. The end state is unchanged (each other main view still ends at origin 0, and the destination always re-renders), but the brief placeholder now stays at the source view's real scroll position until the real content paints. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/gui/main_panels.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/pkg/gui/main_panels.go b/pkg/gui/main_panels.go index 03b7469d2..a0efcb14c 100644 --- a/pkg/gui/main_panels.go +++ b/pkg/gui/main_panels.go @@ -107,16 +107,6 @@ func (gui *Gui) allMainContextPairs() []types.MainContextPair { } func (gui *Gui) refreshMainViews(opts types.RefreshMainOpts) { - // need to reset scroll positions of all other main views - for _, pair := range gui.allMainContextPairs() { - if pair.Main != opts.Pair.Main { - pair.Main.GetView().SetOrigin(0, 0) - } - if pair.Secondary != nil && pair.Secondary != opts.Pair.Secondary { - pair.Secondary.GetView().SetOrigin(0, 0) - } - } - gui.moveMainContextPairToTop(opts.Pair) if opts.Main != nil { @@ -129,6 +119,20 @@ func (gui *Gui) refreshMainViews(opts types.RefreshMainOpts) { opts.Pair.Secondary.GetView().Clear() } + // Reset the scroll positions of all the other main views. We do this after + // moving this pair to the top (which copies the previously-shown view's + // content into the now-visible one to avoid a blank frame): resetting first + // would zero that source view's scroll before it gets copied, forcing the + // placeholder to the top instead of leaving it where the screen already was. + for _, pair := range gui.allMainContextPairs() { + if pair.Main != opts.Pair.Main { + pair.Main.GetView().SetOrigin(0, 0) + } + if pair.Secondary != nil && pair.Secondary != opts.Pair.Secondary { + pair.Secondary.GetView().SetOrigin(0, 0) + } + } + gui.splitMainPanel(opts.Secondary != nil) } From 2a6cb8d78e4fb4959d3ab57c26bf7751ddbdcb63 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 8 Aug 2026 19:12:31 +0200 Subject: [PATCH 10/16] Bundle a view's cell buffer and write state into a viewBuffer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fields that make up a view's content and the act of writing to it — the cell buffer (lines), the write cursor (wx/wy), the escape-sequence decoder (ei) and the held-newline flag (pendingNewline) — were loose fields on View. Bundle them into a viewBuffer struct that View holds by pointer. This is a behaviour-preserving prep refactor: every access just goes through v.buf now. It sets up rendering into a second, off-screen viewBuffer that can be swapped in atomically, so an async re-render never exposes a half-written buffer to readers. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/gocui/gui.go | 8 +- pkg/gocui/view.go | 249 ++++++++++++++++++++++------------------- pkg/gocui/view_test.go | 30 ++--- 3 files changed, 153 insertions(+), 134 deletions(-) diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index ddb356b20..9f40ff17d 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -1497,7 +1497,7 @@ func (g *Gui) drawSubtitle(v *View, fgColor, bgColor Attribute) error { // drawListFooter draws the footer of a list view, showing something like '1 of 10' func (g *Gui) drawListFooter(v *View, fgColor, bgColor Attribute) error { - if len(v.lines) == 0 { + if len(v.buf.lines) == 0 { return nil } @@ -1747,13 +1747,13 @@ func (g *Gui) onKey(ev *GocuiEvent) error { if newY < 0 { newY = 0 newCy = -v.oy - } else if newY >= len(v.lines) { - newY = len(v.lines) - 1 + } else if newY >= len(v.buf.lines) { + newY = len(v.buf.lines) - 1 newCy = newY - v.oy } visibleLineWidth := 0 - for _, c := range v.lines[newY].cells { + for _, c := range v.buf.lines[newY].cells { visibleLineWidth += c.width } if visibleLineWidth < newX { diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index 66d0cde3a..d0af41576 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -25,17 +25,44 @@ const ( RIGHT = 8 // view is overlapping at right edge ) +// viewBuffer holds a view's content as cells, together with the cursor and +// escape-sequence decoder state used to turn incoming bytes into those cells. +// A view normally has a single buffer (the one it displays), but bundling this +// state lets a re-render build a second, off-screen buffer and swap it in +// atomically once the new content is ready, so no reader ever sees a +// half-written buffer. +type viewBuffer struct { + // the view's content: one []cell per unwrapped line + lines []lineType + + // write cursor into lines + wx, wy int + + // decodes ESC sequences as bytes are written + ei *escapeInterpreter + + // If the last character written was a newline, we don't write it but instead + // set pendingNewline to true. If more text is written, we write the newline + // then. This avoids an extra blank line at the end of the view. + pendingNewline bool +} + // A View is a window. It maintains its own internal buffer and cursor // position. type View struct { name string - x0, y0, x1, y1 int // left top right bottom - ox, oy int // view offsets - cx, cy int // cursor position - rx, ry int // Read() offsets - wx, wy int // Write() offsets - lines []lineType // All the data + x0, y0, x1, y1 int // left top right bottom + ox, oy int // view offsets + cx, cy int // cursor position + rx, ry int // Read() offsets outMode OutputMode + + // buf bundles the view's cell buffer and the cursor / escape-parser state + // used to write into it (see the viewBuffer type). Bundling these makes it + // possible to build a second, off-screen buffer during a re-render and swap + // it in atomically once ready, so no reader ever sees a half-written buffer. + buf *viewBuffer + // The y position of the first line of a range selection. // This is not relative to the view's origin: it is relative to the first line // of the view's content, so you can scroll the view and this value will remain @@ -74,17 +101,9 @@ type View struct { // true and viewLines to nil viewLines []viewLine - // If the last character written was a newline, we don't write it but - // instead set pendingNewline to true. If more text is written, we write the - // newline then. This is to avoid having an extra blank at the end of the view. - pendingNewline bool - // writeMutex protects locks the write process writeMutex sync.Mutex - // ei is used to decode ESC sequences on Write - ei *escapeInterpreter - // Visible specifies whether the view is visible. Visible bool @@ -461,7 +480,7 @@ type SearchPosition struct { } type viewLine struct { - linesX, linesY int // coordinates relative to v.lines + linesX, linesY int // coordinates relative to v.buf.lines line []cell // Colors used to extend the bg past this wrapped segment's content. @@ -470,7 +489,7 @@ type viewLine struct { trailingFillAttributes *trailingFillAttributes } -// lineType is one of v.lines: the cells of a source line, plus optional +// lineType is one of v.buf.lines: the cells of a source line, plus optional // trailingFillAttributes recording the colors used to extend the bg // past the line's content when the writer emitted '\x1b[K'. type lineType struct { @@ -536,7 +555,7 @@ func NewView(name string, x0, y0, x1, y1 int, mode OutputMode) *View { Editor: DefaultEditor, tainted: true, outMode: mode, - ei: newEscapeInterpreter(mode), + buf: &viewBuffer{ei: newEscapeInterpreter(mode)}, searcher: &searcher{}, TextArea: &TextArea{}, rangeSelectStartY: -1, @@ -547,7 +566,7 @@ func NewView(name string, x0, y0, x1, y1 int, mode OutputMode) *View { v.SelFgColor, v.SelBgColor = ColorDefault, ColorDefault v.InactiveViewSelBgColor = ColorDefault v.TitleColor, v.FrameColor = ColorDefault, ColorDefault - v.ei.screenColMax = v.InnerWidth() + v.buf.ei.screenColMax = v.InnerWidth() return v } @@ -558,7 +577,7 @@ func NewView(name string, x0, y0, x1, y1 int, mode OutputMode) *View { // content can consult this snapshot instead of reading the view's live // dimensions (which the UI thread mutates during layout). func (v *View) SetContentWidth(width int) { - v.ei.screenColMax = width + v.buf.ei.screenColMax = width } // Dimensions returns the dimensions of the View @@ -748,16 +767,16 @@ func (v *View) SetWritePos(x, y int) { y = 0 } - v.wx = x - v.wy = y + v.buf.wx = x + v.buf.wy = y // Changing the write position makes a pending newline obsolete - v.pendingNewline = false + v.buf.pendingNewline = false } // WritePos returns the current write position of the view's internal buffer. func (v *View) WritePos() (x, y int) { - return v.wx, v.wy + return v.buf.wx, v.buf.wy } // SetReadPos sets the read position of the view's internal buffer. @@ -785,52 +804,52 @@ func (v *View) makeWriteable(x, y int) { // TODO: make this more efficient // line `y` must be index-able (that's why `<=`) - for len(v.lines) <= y { - if cap(v.lines) > len(v.lines) { - newLen := cap(v.lines) + for len(v.buf.lines) <= y { + if cap(v.buf.lines) > len(v.buf.lines) { + newLen := cap(v.buf.lines) if newLen > y { newLen = y + 1 } - v.lines = v.lines[:newLen] + v.buf.lines = v.buf.lines[:newLen] } else { - v.lines = append(v.lines, lineType{}) + v.buf.lines = append(v.buf.lines, lineType{}) } } // cell `x` need not be index-able (that's why `<`) // append should be used by `lines[y]` user if he wants to write beyond `x` - for len(v.lines[y].cells) < x { - if cap(v.lines[y].cells) > len(v.lines[y].cells) { - newLen := cap(v.lines[y].cells) + for len(v.buf.lines[y].cells) < x { + if cap(v.buf.lines[y].cells) > len(v.buf.lines[y].cells) { + newLen := cap(v.buf.lines[y].cells) if newLen > x { newLen = x } - v.lines[y].cells = v.lines[y].cells[:newLen] + v.buf.lines[y].cells = v.buf.lines[y].cells[:newLen] } else { - v.lines[y].cells = append(v.lines[y].cells, cell{}) + v.buf.lines[y].cells = append(v.buf.lines[y].cells, cell{}) } } } -// writeCells copies []cell to (v.wx, v.wy), and advances v.wx accordingly. +// writeCells copies []cell to (v.buf.wx, v.buf.wy), and advances v.buf.wx accordingly. // !!! caller MUST ensure that specified location (x, y) is writeable by calling makeWriteable func (v *View) writeCells(cells []cell) { var newLen int // use maximum len available - line := v.lines[v.wy].cells[:cap(v.lines[v.wy].cells)] - maxCopy := len(line) - v.wx + line := v.buf.lines[v.buf.wy].cells[:cap(v.buf.lines[v.buf.wy].cells)] + maxCopy := len(line) - v.buf.wx if maxCopy < len(cells) { - copy(line[v.wx:], cells[:maxCopy]) + copy(line[v.buf.wx:], cells[:maxCopy]) line = append(line, cells[maxCopy:]...) newLen = len(line) } else { // maxCopy >= len(cells) - copy(line[v.wx:], cells) - newLen = v.wx + len(cells) - if newLen < len(v.lines[v.wy].cells) { - newLen = len(v.lines[v.wy].cells) + copy(line[v.buf.wx:], cells) + newLen = v.buf.wx + len(cells) + if newLen < len(v.buf.lines[v.buf.wy].cells) { + newLen = len(v.buf.lines[v.buf.wy].cells) } } - v.lines[v.wy].cells = line[:newLen] - v.wx += len(cells) + v.buf.lines[v.buf.wy].cells = line[:newLen] + v.buf.wx += len(cells) } // Write appends a byte slice into the view's internal buffer. Because @@ -848,35 +867,35 @@ func (v *View) Write(p []byte) (n int, err error) { func (v *View) write(p []byte) { v.tainted = true - // write only ever touches lines from v.wy onwards, so any cached wrapping + // write only ever touches lines from v.buf.wy onwards, so any cached wrapping // below that stays valid. - v.firstDirtyLine = min(v.firstDirtyLine, v.wy) + v.firstDirtyLine = min(v.firstDirtyLine, v.buf.wy) v.clearHover() // Fill with empty cells, if writing outside current view buffer - v.makeWriteable(v.wx, v.wy) + v.makeWriteable(v.buf.wx, v.buf.wy) finishLine := func() { v.autoRenderHyperlinksInCurrentLine() } advanceToNextLine := func() { - v.wx = 0 - v.wy++ - if v.wy >= len(v.lines) { - v.lines = append(v.lines, lineType{}) + v.buf.wx = 0 + v.buf.wy++ + if v.buf.wy >= len(v.buf.lines) { + v.buf.lines = append(v.buf.lines, lineType{}) } } - if v.pendingNewline { + if v.buf.pendingNewline { advanceToNextLine() - v.ei.notifyRowAdvance() - v.pendingNewline = false + v.buf.ei.notifyRowAdvance() + v.buf.pendingNewline = false } until := len(p) if !v.Editable && until > 0 && p[until-1] == '\n' { - v.pendingNewline = true + v.buf.pendingNewline = true until-- } @@ -892,15 +911,15 @@ func (v *View) write(p []byte) { case characterEquals(chr, '\n') || isCRLF(chr): finishLine() advanceToNextLine() - v.ei.notifyRowAdvance() + v.buf.ei.notifyRowAdvance() case characterEquals(chr, '\r'): finishLine() - v.wx = 0 - v.ei.notifyColumnReset() + v.buf.wx = 0 + v.buf.ei.notifyColumnReset() default: - truncateLine, cells := v.parseInput(chr, width, v.wx, v.wy) - if cd, ok := v.ei.instruction.(cursorDown); ok { - v.ei.instructionRead() + truncateLine, cells := v.parseInput(chr, width, v.buf.wx, v.buf.wy) + if cd, ok := v.buf.ei.instruction.(cursorDown); ok { + v.buf.ei.instructionRead() for range cd.n { v.autoRenderHyperlinksInCurrentLine() advanceToNextLine() @@ -911,7 +930,7 @@ func (v *View) write(p []byte) { } v.writeCells(cells) if truncateLine { - v.lines[v.wy].cells = v.lines[v.wy].cells[:v.wx] + v.buf.lines[v.buf.wy].cells = v.buf.lines[v.buf.wy].cells[:v.buf.wx] } // Soft-wrap tracking. truncateLine is true exactly when the // cells are from \x1b[K filling to end of line — ConPTY @@ -922,12 +941,12 @@ func (v *View) write(p []byte) { for _, c := range cells { totalWidth += c.width } - v.ei.notifyCellsWritten(totalWidth) + v.buf.ei.notifyCellsWritten(totalWidth) } } } - if v.pendingNewline { + if v.buf.pendingNewline { finishLine() } else { v.autoRenderHyperlinksInCurrentLine() @@ -981,7 +1000,7 @@ func (v *View) autoRenderHyperlinksInCurrentLine() { return } - line := v.lines[v.wy].cells + line := v.buf.lines[v.buf.wy].cells start := 0 for { linkStart := findLinkStart(line[start:]) @@ -998,7 +1017,7 @@ func (v *View) autoRenderHyperlinksInCurrentLine() { link.WriteString(line[linkEnd].chr) } for i := linkStart; i < linkEnd; i++ { - v.lines[v.wy].cells[i].hyperlink = link.String() + v.buf.lines[v.buf.wy].cells[i].hyperlink = link.String() } start = linkEnd } @@ -1011,9 +1030,9 @@ func (v *View) parseInput(ch []byte, width int, x int, _ int) (bool, []cell) { cells := []cell{} truncateLine := false - isEscape, err := v.ei.parseOne(ch) + isEscape, err := v.buf.ei.parseOne(ch) if err != nil { - for _, chr := range v.ei.characters() { + for _, chr := range v.buf.ei.characters() { c := cell{ fgColor: v.FgColor, bgColor: v.BgColor, @@ -1022,28 +1041,28 @@ func (v *View) parseInput(ch []byte, width int, x int, _ int) (bool, []cell) { } cells = append(cells, c) } - v.ei.reset() + v.buf.ei.reset() } else { repeatCount := 1 - if _, ok := v.ei.instruction.(eraseInLineFromCursor); ok { + if _, ok := v.buf.ei.instruction.(eraseInLineFromCursor); ok { // Discard any old content past the cursor and record the // fill colors so draw() paints the trailing area with them. // This extends the bg to the right edge in both the // content-fits and content-wraps cases — for the latter, // the metadata is what reaches every wrapped segment past // the last word. - v.ei.instructionRead() + v.buf.ei.instructionRead() truncateLine = true - v.lines[v.wy].trailingFillAttributes = &trailingFillAttributes{ - fg: v.ei.curFgColor, - bg: v.ei.curBgColor, + v.buf.lines[v.buf.wy].trailingFillAttributes = &trailingFillAttributes{ + fg: v.buf.ei.curFgColor, + bg: v.buf.ei.curBgColor, } return truncateLine, []cell{} - } else if cf, ok := v.ei.instruction.(cursorForward); ok { + } else if cf, ok := v.buf.ei.instruction.(cursorForward); ok { // emit `n` space cells under the parser-tracked SGR — used // to materialize ConPTY's compressed runs of spaces (which // it emits as ECH+CUF instead of literal whitespace). - v.ei.instructionRead() + v.buf.ei.instructionRead() repeatCount = cf.n ch = []byte{' '} width = 1 @@ -1061,9 +1080,9 @@ func (v *View) parseInput(ch []byte, width int, x int, _ int) (bool, []cell) { repeatCount = tabWidth - (x % tabWidth) } c := cell{ - fgColor: v.ei.curFgColor, - bgColor: v.ei.curBgColor, - hyperlink: v.ei.hyperlink.String(), + fgColor: v.buf.ei.curFgColor, + bgColor: v.buf.ei.curBgColor, + hyperlink: v.buf.ei.hyperlink.String(), chr: string(ch), width: width, } @@ -1091,9 +1110,9 @@ func (v *View) Read(p []byte) (n int, err error) { } v.readBuffer = nil } - for v.ry < len(v.lines) { - for v.rx < len(v.lines[v.ry].cells) { - s := v.lines[v.ry].cells[v.rx].chr + for v.ry < len(v.buf.lines) { + for v.rx < len(v.buf.lines[v.ry].cells) { + s := v.buf.lines[v.ry].cells[v.rx].chr count := len(s) copy(p[offset:], s) v.rx++ @@ -1115,7 +1134,7 @@ func (v *View) Read(p []byte) (n int, err error) { // only use this if the calling function has a lock on writeMutex func (v *View) clear() { v.rewind() - v.lines = nil + v.buf.lines = nil v.clearViewLines() } @@ -1157,7 +1176,7 @@ func (v *View) CopyContent(from *View) { // This is a shallow clone -- the per-row cell data is immutable once written // and stays shared, so the cost is proportional to the number of rows, not // their contents. - v.lines = slices.Clone(from.lines) + v.buf.lines = slices.Clone(from.buf.lines) v.viewLines = slices.Clone(from.viewLines) v.SetOriginX(from.ox) v.SetOriginY(from.oy) @@ -1180,13 +1199,13 @@ func (v *View) Reset() { defer v.writeMutex.Unlock() v.rewind() - v.lines = nil + v.buf.lines = nil } // This is for when we've done a restart for the sake of avoiding a flicker and // we've reached the end of the new content to display: we need to clear the remaining // content from the previous round. We do this by setting v.viewLines to nil so that -// we just render the new content from v.lines directly +// we just render the new content from v.buf.lines directly func (v *View) FlushStaleCells() { v.writeMutex.Lock() defer v.writeMutex.Unlock() @@ -1195,8 +1214,8 @@ func (v *View) FlushStaleCells() { } func (v *View) rewind() { - v.ei.reset() - v.ei.resetScreenCursor() + v.buf.ei.reset() + v.buf.ei.resetScreenCursor() v.SetReadPos(0, 0) v.SetWritePos(0, 0) @@ -1268,14 +1287,14 @@ func (v *View) updateSearchPositions() { for _, result := range v.searcher.modelSearchResults { // This code only works when v.Wrap is false. - if result.Y >= len(v.lines) { + if result.Y >= len(v.buf.lines) { break } // If a view line exists for this line index: - if v.lines[result.Y].cells != nil { + if v.buf.lines[result.Y].cells != nil { // search this view line for the search string - positions := searchPositionsForLine(v.lines[result.Y].cells, result.Y) + positions := searchPositionsForLine(v.buf.lines[result.Y].cells, result.Y) if len(positions) > 0 { // If we found any occurrences, add them v.searcher.searchPositions = append(v.searcher.searchPositions, positions...) @@ -1422,7 +1441,7 @@ func (v *View) refreshViewLinesIfNeeded() { } lineIdx := 0 - lines := v.lines + lines := v.buf.lines for i := range lines { line := &lines[i] @@ -1546,8 +1565,8 @@ func (v *View) BufferLines() []string { v.writeMutex.Lock() defer v.writeMutex.Unlock() - lines := make([]string, len(v.lines)) - for i, l := range v.lines { + lines := make([]string, len(v.buf.lines)) + for i, l := range v.buf.lines { lines[i] = l.cells.String() } return lines @@ -1559,7 +1578,7 @@ func (v *View) Buffer() string { v.writeMutex.Lock() defer v.writeMutex.Unlock() - return linesToString(v.lines) + return linesToString(v.buf.lines) } // ViewBufferLines returns the lines in the view's internal @@ -1579,7 +1598,7 @@ func (v *View) ViewBufferLines() []string { // LinesHeight is the count of view lines (i.e. lines excluding wrapping) func (v *View) LinesHeight() int { - return len(v.lines) + return len(v.buf.lines) } // ViewLinesHeight is the count of view lines (i.e. lines including wrapping) @@ -1610,11 +1629,11 @@ func (v *View) Line(y int) (string, bool) { return "", false } - if y < 0 || y >= len(v.lines) { + if y < 0 || y >= len(v.buf.lines) { return "", false } - return v.lines[y].cells.String(), true + return v.buf.lines[y].cells.String(), true } // Word returns a string with the word of the view's internal buffer @@ -1625,11 +1644,11 @@ func (v *View) Word(x, y int) (string, bool) { return "", false } - if x < 0 || y < 0 || y >= len(v.lines) || x >= len(v.lines[y].cells) { + if x < 0 || y < 0 || y >= len(v.buf.lines) || x >= len(v.buf.lines[y].cells) { return "", false } - str := v.lines[y].cells.String() + str := v.buf.lines[y].cells.String() nl := strings.LastIndexFunc(str[:x], indexFunc) if nl == -1 { @@ -1655,12 +1674,12 @@ func indexFunc(r rune) bool { // SetHighlight toggles highlighting of separate lines, for custom lists // or multiple selection in views. func (v *View) SetHighlight(y int, on bool) { - if y < 0 || y >= len(v.lines) { + if y < 0 || y >= len(v.buf.lines) { return } - cells := make([]cell, 0, len(v.lines[y].cells)) - for _, c := range v.lines[y].cells { + cells := make([]cell, 0, len(v.buf.lines[y].cells)) + for _, c := range v.buf.lines[y].cells { if on { c.bgColor = v.SelBgColor c.fgColor = v.SelFgColor @@ -1672,7 +1691,7 @@ func (v *View) SetHighlight(y int, on bool) { } v.tainted = true v.firstDirtyLine = min(v.firstDirtyLine, y) - v.lines[y].cells = cells + v.buf.lines[y].cells = cells v.clearHover() } @@ -1784,7 +1803,7 @@ func (v *View) SelectedLine() string { v.writeMutex.Lock() defer v.writeMutex.Unlock() - if len(v.lines) == 0 { + if len(v.buf.lines) == 0 { return "" } @@ -1796,7 +1815,7 @@ func (v *View) SelectedLines() []string { v.writeMutex.Lock() defer v.writeMutex.Unlock() - if len(v.lines) == 0 { + if len(v.buf.lines) == 0 { return nil } @@ -1811,7 +1830,7 @@ func (v *View) SelectedLines() []string { } func (v *View) lineContentAtIdx(idx int) string { - return v.lines[idx].cells.String() + return v.buf.lines[idx].cells.String() } func (v *View) SelectedPoint() (int, int) { @@ -1884,8 +1903,8 @@ func (v *View) ClearTextArea() { func (v *View) overwriteLines(y int, content string) { // break by newline, then for each line, write it, then add that erase command - v.wx = 0 - v.wy = y + v.buf.wx = 0 + v.buf.wy = y v.clearViewLines() lines := strings.ReplaceAll(content, "\n", "\x1b[K\n") @@ -1897,7 +1916,7 @@ func (v *View) overwriteLines(y int, content string) { v.writeString(lines) } -// only call this function if you don't care where v.wx and v.wy end up +// only call this function if you don't care where v.buf.wx and v.buf.wy end up func (v *View) OverwriteLines(y int, content string) { v.writeMutex.Lock() defer v.writeMutex.Unlock() @@ -1905,7 +1924,7 @@ func (v *View) OverwriteLines(y int, content string) { v.overwriteLines(y, content) } -// only call this function if you don't care where v.wx and v.wy end up +// only call this function if you don't care where v.buf.wx and v.buf.wy end up func (v *View) OverwriteLinesAndClearEverythingElse(lineCount int, y int, content string) { v.writeMutex.Lock() defer v.writeMutex.Unlock() @@ -1915,11 +1934,11 @@ func (v *View) OverwriteLinesAndClearEverythingElse(lineCount int, y int, conten v.overwriteLines(y, content) for i := range y { - v.lines[i] = lineType{} + v.buf.lines[i] = lineType{} } - for i := v.wy + 1; i < len(v.lines); i += 1 { - v.lines[i] = lineType{} + for i := v.buf.wy + 1; i < len(v.buf.lines); i += 1 { + v.buf.lines[i] = lineType{} } } @@ -1927,7 +1946,7 @@ func (v *View) setContentLineCount(lineCount int) { if lineCount > 0 { v.makeWriteable(0, lineCount-1) } - v.lines = v.lines[:lineCount] + v.buf.lines = v.buf.lines[:lineCount] } // If the current search result is no longer visible after a scroll up, select the last search @@ -2061,7 +2080,7 @@ func (v *View) scrollMargin() int { // Returns true if the view contains a line containing the given text with the given // foreground color func (v *View) ContainsColoredText(fgColor string, text string) bool { - for _, line := range v.lines { + for _, line := range v.buf.lines { if containsColoredTextInLine(fgColor, text, line.cells) { return true } diff --git a/pkg/gocui/view_test.go b/pkg/gocui/view_test.go index 121f656d8..61dc26b52 100644 --- a/pkg/gocui/view_test.go +++ b/pkg/gocui/view_test.go @@ -101,13 +101,13 @@ func TestWriteString(t *testing.T) { for _, test := range tests { v := NewView("name", 0, 0, 10, 10, OutputNormal) for _, l := range test.existingLines { - v.lines = append(v.lines, lineType{cells: stringToCells(l)}) + v.buf.lines = append(v.buf.lines, lineType{cells: stringToCells(l)}) } for _, s := range test.stringsToWrite { v.writeString(s) } var resultingLines [][]string - for _, l := range v.lines { + for _, l := range v.buf.lines { resultingLines = append(resultingLines, cellsToStrings(l.cells)) } assert.Equal(t, test.expectedLines, resultingLines) @@ -144,19 +144,19 @@ func TestAutoRenderingHyperlinks(t *testing.T) { v.writeString("htt") // No hyperlinks are generated for incomplete URLs - assert.Equal(t, "", v.lines[0].cells[0].hyperlink) + assert.Equal(t, "", v.buf.lines[0].cells[0].hyperlink) // Writing more characters to the same line makes the link complete (even // though we didn't see a newline yet) v.writeString("ps://example.com") - assert.Equal(t, "https://example.com", v.lines[0].cells[0].hyperlink) + assert.Equal(t, "https://example.com", v.buf.lines[0].cells[0].hyperlink) v.Clear() // Valid but incomplete URL v.writeString("https://exa") - assert.Equal(t, "https://exa", v.lines[0].cells[0].hyperlink) + assert.Equal(t, "https://exa", v.buf.lines[0].cells[0].hyperlink) // Writing more characters to the same fixes the link v.writeString("mple.com") - assert.Equal(t, "https://example.com", v.lines[0].cells[0].hyperlink) + assert.Equal(t, "https://example.com", v.buf.lines[0].cells[0].hyperlink) } func TestContainsColoredText(t *testing.T) { @@ -233,7 +233,7 @@ func TestContainsColoredText(t *testing.T) { for j, cells := range test.lines { lines[j] = lineType{cells: cells} } - v := &View{lines: lines} + v := &View{buf: &viewBuffer{lines: lines}} assert.Equal(t, test.expected, v.ContainsColoredText(test.fgColorStr, test.text), "Test %d failed", i) } } @@ -248,8 +248,8 @@ func TestWriteCursorPositionEscape(t *testing.T) { // "a", then "skip to row 3" (i.e. one blank row), then "b". v.writeString("a\r\n\x1b[3;1Hb\r\n") - got := make([][]string, 0, len(v.lines)) - for _, l := range v.lines { + got := make([][]string, 0, len(v.buf.lines)) + for _, l := range v.buf.lines { got = append(got, cellsToStrings(l.cells)) } @@ -269,8 +269,8 @@ func TestWriteCursorPositionEscapeAcrossWrites(t *testing.T) { // ConPTY is on row 3 here; CUP to row 5 should skip exactly one row. v.writeString("c\x1b[5;1Hd\n") - got := make([][]string, 0, len(v.lines)) - for _, l := range v.lines { + got := make([][]string, 0, len(v.buf.lines)) + for _, l := range v.buf.lines { got = append(got, cellsToStrings(l.cells)) } assert.Equal(t, [][]string{ @@ -292,8 +292,8 @@ func TestWriteCursorForwardEscape(t *testing.T) { // "a" + ECH 5 + CUF 5 + "b" — visually "a b". v.writeString("a\x1b[5X\x1b[5Cb\n") - got := make([][]string, 0, len(v.lines)) - for _, l := range v.lines { + got := make([][]string, 0, len(v.buf.lines)) + for _, l := range v.buf.lines { got = append(got, cellsToStrings(l.cells)) } @@ -312,8 +312,8 @@ func TestWriteCursorPositionEscapeWithSoftWraps(t *testing.T) { v.writeString("abcdefghij\n") v.writeString("\x1b[4;1Hxyz\n") - got := make([][]string, 0, len(v.lines)) - for _, l := range v.lines { + got := make([][]string, 0, len(v.buf.lines)) + for _, l := range v.buf.lines { got = append(got, cellsToStrings(l.cells)) } assert.Equal(t, [][]string{ From cc5d5057a71dbac887d9e2ec03c67d4816efe4d8 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 8 Aug 2026 19:15:16 +0200 Subject: [PATCH 11/16] Make the buffer-writing methods operate on a viewBuffer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit write, writeCells, makeWriteable, parseInput and autoRenderHyperlinksInCurrentLine produced cells into v.buf; move them onto viewBuffer so they can write into any buffer, not just the displayed one. The display-side effects that don't belong to content production — tainting, clearing hover, updating search positions — stay behind in the View.write wrapper, which delegates the actual writing to v.buf.write(v). Render config the writer needs (Editable, colors, width, tab width, hyperlink auto-render) is read from the passed View. Behaviour-preserving: the wrapper still always targets v.buf. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/gocui/view.go | 136 ++++++++++++++++++++++++---------------------- 1 file changed, 72 insertions(+), 64 deletions(-) diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index d0af41576..4d9998529 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -800,56 +800,56 @@ func (v *View) ReadPos() (x, y int) { } // makeWriteable creates empty cells if required to make position (x, y) writeable. -func (v *View) makeWriteable(x, y int) { +func (b *viewBuffer) makeWriteable(x, y int) { // TODO: make this more efficient // line `y` must be index-able (that's why `<=`) - for len(v.buf.lines) <= y { - if cap(v.buf.lines) > len(v.buf.lines) { - newLen := cap(v.buf.lines) + for len(b.lines) <= y { + if cap(b.lines) > len(b.lines) { + newLen := cap(b.lines) if newLen > y { newLen = y + 1 } - v.buf.lines = v.buf.lines[:newLen] + b.lines = b.lines[:newLen] } else { - v.buf.lines = append(v.buf.lines, lineType{}) + b.lines = append(b.lines, lineType{}) } } // cell `x` need not be index-able (that's why `<`) // append should be used by `lines[y]` user if he wants to write beyond `x` - for len(v.buf.lines[y].cells) < x { - if cap(v.buf.lines[y].cells) > len(v.buf.lines[y].cells) { - newLen := cap(v.buf.lines[y].cells) + for len(b.lines[y].cells) < x { + if cap(b.lines[y].cells) > len(b.lines[y].cells) { + newLen := cap(b.lines[y].cells) if newLen > x { newLen = x } - v.buf.lines[y].cells = v.buf.lines[y].cells[:newLen] + b.lines[y].cells = b.lines[y].cells[:newLen] } else { - v.buf.lines[y].cells = append(v.buf.lines[y].cells, cell{}) + b.lines[y].cells = append(b.lines[y].cells, cell{}) } } } -// writeCells copies []cell to (v.buf.wx, v.buf.wy), and advances v.buf.wx accordingly. +// writeCells copies []cell to (b.wx, b.wy), and advances b.wx accordingly. // !!! caller MUST ensure that specified location (x, y) is writeable by calling makeWriteable -func (v *View) writeCells(cells []cell) { +func (b *viewBuffer) writeCells(cells []cell) { var newLen int // use maximum len available - line := v.buf.lines[v.buf.wy].cells[:cap(v.buf.lines[v.buf.wy].cells)] - maxCopy := len(line) - v.buf.wx + line := b.lines[b.wy].cells[:cap(b.lines[b.wy].cells)] + maxCopy := len(line) - b.wx if maxCopy < len(cells) { - copy(line[v.buf.wx:], cells[:maxCopy]) + copy(line[b.wx:], cells[:maxCopy]) line = append(line, cells[maxCopy:]...) newLen = len(line) } else { // maxCopy >= len(cells) - copy(line[v.buf.wx:], cells) - newLen = v.buf.wx + len(cells) - if newLen < len(v.buf.lines[v.buf.wy].cells) { - newLen = len(v.buf.lines[v.buf.wy].cells) + copy(line[b.wx:], cells) + newLen = b.wx + len(cells) + if newLen < len(b.lines[b.wy].cells) { + newLen = len(b.lines[b.wy].cells) } } - v.buf.lines[v.buf.wy].cells = line[:newLen] - v.buf.wx += len(cells) + b.lines[b.wy].cells = line[:newLen] + b.wx += len(cells) } // Write appends a byte slice into the view's internal buffer. Because @@ -872,30 +872,40 @@ func (v *View) write(p []byte) { v.firstDirtyLine = min(v.firstDirtyLine, v.buf.wy) v.clearHover() + v.buf.write(v, p) + + v.updateSearchPositions() +} + +// write parses p into cells and appends them to the buffer at its write cursor. +// It only touches the buffer; the View wrapper above handles display-side +// effects (tainting, hover, search). v supplies render config (Editable, colors, +// width, tab width, hyperlink auto-rendering). +func (b *viewBuffer) write(v *View, p []byte) { // Fill with empty cells, if writing outside current view buffer - v.makeWriteable(v.buf.wx, v.buf.wy) + b.makeWriteable(b.wx, b.wy) finishLine := func() { - v.autoRenderHyperlinksInCurrentLine() + b.autoRenderHyperlinksInCurrentLine(v) } advanceToNextLine := func() { - v.buf.wx = 0 - v.buf.wy++ - if v.buf.wy >= len(v.buf.lines) { - v.buf.lines = append(v.buf.lines, lineType{}) + b.wx = 0 + b.wy++ + if b.wy >= len(b.lines) { + b.lines = append(b.lines, lineType{}) } } - if v.buf.pendingNewline { + if b.pendingNewline { advanceToNextLine() - v.buf.ei.notifyRowAdvance() - v.buf.pendingNewline = false + b.ei.notifyRowAdvance() + b.pendingNewline = false } until := len(p) if !v.Editable && until > 0 && p[until-1] == '\n' { - v.buf.pendingNewline = true + b.pendingNewline = true until-- } @@ -911,26 +921,26 @@ func (v *View) write(p []byte) { case characterEquals(chr, '\n') || isCRLF(chr): finishLine() advanceToNextLine() - v.buf.ei.notifyRowAdvance() + b.ei.notifyRowAdvance() case characterEquals(chr, '\r'): finishLine() - v.buf.wx = 0 - v.buf.ei.notifyColumnReset() + b.wx = 0 + b.ei.notifyColumnReset() default: - truncateLine, cells := v.parseInput(chr, width, v.buf.wx, v.buf.wy) - if cd, ok := v.buf.ei.instruction.(cursorDown); ok { - v.buf.ei.instructionRead() + truncateLine, cells := b.parseInput(v, chr, width, b.wx, b.wy) + if cd, ok := b.ei.instruction.(cursorDown); ok { + b.ei.instructionRead() for range cd.n { - v.autoRenderHyperlinksInCurrentLine() + b.autoRenderHyperlinksInCurrentLine(v) advanceToNextLine() } } if cells == nil { continue } - v.writeCells(cells) + b.writeCells(cells) if truncateLine { - v.buf.lines[v.buf.wy].cells = v.buf.lines[v.buf.wy].cells[:v.buf.wx] + b.lines[b.wy].cells = b.lines[b.wy].cells[:b.wx] } // Soft-wrap tracking. truncateLine is true exactly when the // cells are from \x1b[K filling to end of line — ConPTY @@ -941,18 +951,16 @@ func (v *View) write(p []byte) { for _, c := range cells { totalWidth += c.width } - v.buf.ei.notifyCellsWritten(totalWidth) + b.ei.notifyCellsWritten(totalWidth) } } } - if v.buf.pendingNewline { + if b.pendingNewline { finishLine() } else { - v.autoRenderHyperlinksInCurrentLine() + b.autoRenderHyperlinksInCurrentLine(v) } - - v.updateSearchPositions() } // exported functions use the mutex. Non-exported functions are for internal use @@ -995,12 +1003,12 @@ var lineEndCharacters = map[string]bool{ ")": true, } -func (v *View) autoRenderHyperlinksInCurrentLine() { +func (b *viewBuffer) autoRenderHyperlinksInCurrentLine(v *View) { if !v.AutoRenderHyperLinks { return } - line := v.buf.lines[v.buf.wy].cells + line := b.lines[b.wy].cells start := 0 for { linkStart := findLinkStart(line[start:]) @@ -1017,7 +1025,7 @@ func (v *View) autoRenderHyperlinksInCurrentLine() { link.WriteString(line[linkEnd].chr) } for i := linkStart; i < linkEnd; i++ { - v.buf.lines[v.buf.wy].cells[i].hyperlink = link.String() + b.lines[b.wy].cells[i].hyperlink = link.String() } start = linkEnd } @@ -1026,13 +1034,13 @@ func (v *View) autoRenderHyperlinksInCurrentLine() { // parseInput parses char by char the input written to the View. It returns nil // while processing ESC sequences. Otherwise, it returns a cell slice that // contains the processed data. -func (v *View) parseInput(ch []byte, width int, x int, _ int) (bool, []cell) { +func (b *viewBuffer) parseInput(v *View, ch []byte, width int, x int, _ int) (bool, []cell) { cells := []cell{} truncateLine := false - isEscape, err := v.buf.ei.parseOne(ch) + isEscape, err := b.ei.parseOne(ch) if err != nil { - for _, chr := range v.buf.ei.characters() { + for _, chr := range b.ei.characters() { c := cell{ fgColor: v.FgColor, bgColor: v.BgColor, @@ -1041,28 +1049,28 @@ func (v *View) parseInput(ch []byte, width int, x int, _ int) (bool, []cell) { } cells = append(cells, c) } - v.buf.ei.reset() + b.ei.reset() } else { repeatCount := 1 - if _, ok := v.buf.ei.instruction.(eraseInLineFromCursor); ok { + if _, ok := b.ei.instruction.(eraseInLineFromCursor); ok { // Discard any old content past the cursor and record the // fill colors so draw() paints the trailing area with them. // This extends the bg to the right edge in both the // content-fits and content-wraps cases — for the latter, // the metadata is what reaches every wrapped segment past // the last word. - v.buf.ei.instructionRead() + b.ei.instructionRead() truncateLine = true - v.buf.lines[v.buf.wy].trailingFillAttributes = &trailingFillAttributes{ - fg: v.buf.ei.curFgColor, - bg: v.buf.ei.curBgColor, + b.lines[b.wy].trailingFillAttributes = &trailingFillAttributes{ + fg: b.ei.curFgColor, + bg: b.ei.curBgColor, } return truncateLine, []cell{} - } else if cf, ok := v.buf.ei.instruction.(cursorForward); ok { + } else if cf, ok := b.ei.instruction.(cursorForward); ok { // emit `n` space cells under the parser-tracked SGR — used // to materialize ConPTY's compressed runs of spaces (which // it emits as ECH+CUF instead of literal whitespace). - v.buf.ei.instructionRead() + b.ei.instructionRead() repeatCount = cf.n ch = []byte{' '} width = 1 @@ -1080,9 +1088,9 @@ func (v *View) parseInput(ch []byte, width int, x int, _ int) (bool, []cell) { repeatCount = tabWidth - (x % tabWidth) } c := cell{ - fgColor: v.buf.ei.curFgColor, - bgColor: v.buf.ei.curBgColor, - hyperlink: v.buf.ei.hyperlink.String(), + fgColor: b.ei.curFgColor, + bgColor: b.ei.curBgColor, + hyperlink: b.ei.hyperlink.String(), chr: string(ch), width: width, } @@ -1944,7 +1952,7 @@ func (v *View) OverwriteLinesAndClearEverythingElse(lineCount int, y int, conten func (v *View) setContentLineCount(lineCount int) { if lineCount > 0 { - v.makeWriteable(0, lineCount-1) + v.buf.makeWriteable(0, lineCount-1) } v.buf.lines = v.buf.lines[:lineCount] } From 87b30d9581fcdea1d24c89900023319a4904c2fe Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 9 Aug 2026 11:25:52 +0200 Subject: [PATCH 12/16] Don't take the view over for "loading..." when the content isn't changing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A render that takes more than 200ms to produce its first line takes the view over to say "loading...", which clears the buffer it was showing. That is worth doing when the content coming is different — the view is showing something the user has moved on from, and saying so beats leaving it there silently. It is pure flicker when the content isn't changing: the view is already showing exactly what the render is about to put back, and a slow re-render of unchanged content is common (a background refresh over a repo with submodules that have uncommitted changes, say). So track whether the render in flight has content the view isn't already showing, and only let the indicator take over when it does. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/tasks/tasks.go | 25 ++++++++++++-- pkg/tasks/tasks_test.go | 75 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index a2084ea36..0fa04eb3e 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -78,6 +78,13 @@ type ViewBufferManager struct { taskKey string onNewKey func() + // Whether the content the running task is rendering differs from what the + // view is currently showing (i.e. the command key changed). The loading + // indicator only takes the view over when it is set: there is no point + // clearing content we are about to render identically. Cleared once the + // task has rendered enough for the view to be showing the new content. + newContentPending atomic.Bool + // Whether a command task is currently reading content into the view. While // this is true the content is still growing, so callers (e.g. the layout) // must not clamp the view's scroll position to the amount loaded so far. @@ -300,7 +307,14 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix return case <-ticker.C: loadingMutex.Lock() - if !loaded { + // Only take the view over to say "loading..." when the content coming + // is different from what's on screen. A re-render of the same content + // leaves the view showing exactly what it should already, so clearing + // it for the message and then rendering the same thing back is a + // visible flicker for nothing — and a slow re-render of unchanged + // content is common (a background refresh over a repo with submodules + // that have uncommitted changes, say). + if !loaded && self.newContentPending.Load() { self.beforeStart() _, _ = self.writer.Write([]byte("loading...")) self.refreshView() @@ -399,6 +413,8 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix // whether to scroll) and sets the origin, both of which // are UI-thread-only, so run it there. _ = self.onUIThread(self.onEndOfInput) + // Whatever there was to show is on screen now. + self.newContentPending.Store(false) // The content is fully loaded now, so it's safe again for the // layout to clamp the scroll position to it. We deliberately // don't clear this when stopped (rather than EOF'd), because that @@ -434,6 +450,7 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix // We have read enough lines to fill the view, so do a first refresh // here to show what we have. Continue reading and refresh again at // the end to make sure the scrollbar has the right size. + self.newContentPending.Store(false) refreshViewIfStale() } } @@ -554,7 +571,11 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error // Read taskKey directly: we already hold the mutex that guards it, and // GetTaskKey would take it again. - resetOrigin := self.taskKey != key && self.onNewKey != nil + newContent := self.taskKey != key + if newContent { + self.newContentPending.Store(true) + } + resetOrigin := newContent && self.onNewKey != nil self.taskKey = key self.taskIDMutex.Unlock() diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index d9ec7e4d4..ad4469043 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -7,6 +7,7 @@ import ( "reflect" "strings" "sync" + "sync/atomic" "testing" "time" @@ -251,6 +252,80 @@ func TestNewCmdTaskQueuedReadAtEndOfInput(t *testing.T) { assert.True(t, thenCalled) } +// A writer that records whether the loading indicator was ever written to it. +type LoadingIndicatorSpy struct { + sawLoadingIndicator atomic.Bool +} + +func (self *LoadingIndicatorSpy) Write(p []byte) (n int, err error) { + if bytes.Contains(p, []byte("loading...")) { + self.sawLoadingIndicator.Store(true) + } + return len(p), nil +} + +// A render that takes long enough to produce its first line takes the view over +// to say "loading...", which means clearing whatever it was showing. That is only +// worth doing when the content coming is different from what's on screen: +// re-rendering the same content would otherwise clear the view and render the +// same thing straight back, a visible flicker for nothing. +func TestLoadingIndicatorOnlyTakesOverForNewContent(t *testing.T) { + writer := &LoadingIndicatorSpy{} + + manager := NewViewBufferManager( + utils.NewDummyLog(), + writer, + func() {}, + func() {}, + func() {}, + func() {}, + func() gocui.Task { return gocui.NewFakeTask() }, + // no UI thread in the test; run the view mutations inline + func(f func()) error { f(); return nil }, + ) + + startTask := func(key string, reader io.Reader, onDone func()) { + start := func() (Cmd, io.Reader) { + // not actually starting this because it's not necessary + return ExecCmd{Cmd: exec.Command("blah")}, reader + } + _ = manager.NewTask(manager.NewCmdTask(start, "", LinesToRead{100, 50, nil}, onDone), key) + } + // Starts a task whose command produces nothing at all, so that it is still + // waiting for its first line when the loading indicator falls due. Returns + // the reader so the caller can let it finish. + startStalledTask := func(key string) *BlockingLineReader { + reader := &BlockingLineReader{ + blocked: make(chan struct{}), + unblock: make(chan struct{}), + } + startTask(key, reader, nil) + <-reader.blocked + return reader + } + + // Get some content on screen first: the indicator is only due when a render + // is slow, and this one isn't. + done := make(chan struct{}) + startTask("cmd1", &BlankLineReader{totalLinesToYield: 3}, func() { close(done) }) + <-done + assert.False(t, writer.sawLoadingIndicator.Load()) + + // A slow re-render of that same content must leave the view alone however + // long it takes. The indicator is due 200ms in, so give it well past that. + sameContent := startStalledTask("cmd1") + defer close(sameContent.unblock) + time.Sleep(500 * time.Millisecond) + assert.False(t, writer.sawLoadingIndicator.Load()) + + // Different content, though, is worth taking the view over for. + newContent := startStalledTask("cmd2") + defer close(newContent.unblock) + assert.Eventually(t, + writer.sawLoadingIndicator.Load, + 2*time.Second, 10*time.Millisecond) +} + func TestNewCmdTaskRefresh(t *testing.T) { type scenario struct { name string From 9e23111172deb1b3b80b39ef6efacb6a7db9e08f Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 8 Aug 2026 22:32:23 +0200 Subject: [PATCH 13/16] Render async content into an off-screen buffer and swap it in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cmd/pty re-render used to overwrite the displayed buffer from the top down as lines arrived, relying on keeping the previous render's view-line tail to avoid a blank frame. That left the view showing a mixture of old and new content while loading, and any reader (draw, clicks, the view-line mapping) could observe a half-written buffer at the wrong scroll. Instead, build the new content in a second, off-screen viewBuffer: until the task has read enough to paint, writes go there and the displayed buffer — and so everything every reader sees — is left untouched. Once the task reaches its first-paint point (InitialRefreshAfter, or EOF for short content) it swaps the off-screen buffer in atomically, so the view jumps straight from the previous render to the new one with no intermediate frame. Subsequent lines append to the now-displayed buffer. Swapping at the first-paint point means the displayed buffer is only a viewport tall when it appears and then grows as the rest streams in toward the count needed for an accurate scrollbar. The scrollbar is sized from the displayed buffer's height, so left to itself the thumb would shrink and snap back during that growth (most visibly: the files panel's periodic refresh making the thumb jump while scrolled down). The total height the scrollbar needs is a strictly later quantity than the viewport-fill paint, so no single early swap can have both right. FreezeScrollbarHeight therefore records the view's height when a load begins and the scrollbar is held there — growing only if the new content turns out taller — until the load ends; a synchronous render superseding the load releases it. This mirrors the layout clamp, which already ignores the partial content height while a view loads. With the swap doing a wholesale replace, refreshViewLinesIfNeeded can truncate the view lines to the current buffer: there is no longer a half-loaded shorter buffer whose tail we must keep showing, so a stale tail never forms. clear()/Reset() abandon any in-progress off-screen render so a synchronous SetContent after a stopped task writes to the display. The swap holds writeMutex for now; it could later move to the main thread. Flicker behaviour still needs interactive verification (LAZYGIT_SLOW_RENDER + a real diff renderer). Co-Authored-By: Claude Opus 5 (1M context) --- pkg/gocui/gui.go | 2 +- pkg/gocui/view.go | 124 ++++++++++++++++++++++++++++++++++++++- pkg/gocui/view_test.go | 121 ++++++++++++++++++++++++++++++++++++++ pkg/gui/pty.go | 4 ++ pkg/gui/tasks_adapter.go | 22 +++++-- pkg/tasks/tasks.go | 39 +++++++++--- pkg/tasks/tasks_test.go | 20 ++++++- 7 files changed, 312 insertions(+), 20 deletions(-) diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index 9f40ff17d..b4cf107de 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -1276,7 +1276,7 @@ func calcScrollbarRune( func calcRealScrollbarStartEnd(v *View) (bool, int, int) { height := v.InnerHeight() - fullHeight := v.ViewLinesHeight() - v.scrollMargin() + fullHeight := v.scrollbarContentHeight() - v.scrollMargin() if v.CanScrollPastBottom { fullHeight += height diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index 4d9998529..fe01e50ca 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -58,11 +58,18 @@ type View struct { outMode OutputMode // buf bundles the view's cell buffer and the cursor / escape-parser state - // used to write into it (see the viewBuffer type). Bundling these makes it - // possible to build a second, off-screen buffer during a re-render and swap - // it in atomically once ready, so no reader ever sees a half-written buffer. + // used to write into it (see the viewBuffer type). It is the buffer every + // reader sees. buf *viewBuffer + // While non-nil, writes go here instead of buf, so an async re-render can + // build its new content without disturbing what readers (draw, clicks, + // scrolling, …) see. The task swaps it into buf once it has read enough to + // paint (SwapInOffscreenRender), so the displayed content jumps straight + // from the previous render to the new one with no half-written frame in + // between. nil during normal (non-async) writes. + offscreen *viewBuffer + // The y position of the first line of a range selection. // This is not relative to the view's origin: it is relative to the first line // of the view's content, so you can scroll the view and this value will remain @@ -101,6 +108,17 @@ type View struct { // true and viewLines to nil viewLines []viewLine + // While a re-render is loading new content (see offscreen), the displayed + // buffer is only partially filled once we've swapped the off-screen render + // in: the task keeps appending lines after the first paint, up to the count + // needed for an accurate scrollbar. Sizing the scrollbar from that partial + // view-line count would make the thumb shrink and snap back as the rest + // streams in. So while a load is in progress we hold the scrollbar's height + // at this value — the height the view had when the load began — and let it + // grow only if the new content turns out taller. Zero means no load is in + // progress and the scrollbar tracks the content directly. + scrollbarHeightFloor int + // writeMutex protects locks the write process writeMutex sync.Mutex @@ -866,6 +884,14 @@ func (v *View) Write(p []byte) (n int, err error) { } func (v *View) write(p []byte) { + // An async re-render builds into the off-screen buffer (see View.offscreen) + // until it swaps in; until then the displayed buffer, and so everything + // readers see, is left untouched. + if v.offscreen != nil { + v.offscreen.write(v, p) + return + } + v.tainted = true // write only ever touches lines from v.buf.wy onwards, so any cached wrapping // below that stays valid. @@ -1144,6 +1170,15 @@ func (v *View) clear() { v.rewind() v.buf.lines = nil v.clearViewLines() + // Abandon any in-progress off-screen render: a synchronous SetContent/Clear + // is taking over the displayed buffer, so writes must go there, not into a + // stale off-screen buffer left by a stopped task. + v.offscreen = nil + // Likewise release any held scrollbar height: the new content is defined + // synchronously (e.g. a string render superseding a still-loading diff), so + // there's no async growth left to smooth over and the scrollbar should track + // the new content directly. + v.scrollbarHeightFloor = 0 } // Clear empties the view's internal buffer. @@ -1208,6 +1243,9 @@ func (v *View) Reset() { v.rewind() v.buf.lines = nil + // As in clear(): abandon any in-progress off-screen render so writes after a + // reset go to the displayed buffer. + v.offscreen = nil } // This is for when we've done a restart for the sake of avoiding a flicker and @@ -1221,6 +1259,79 @@ func (v *View) FlushStaleCells() { v.clearViewLines() } +// BeginOffscreenRender starts building a re-render into an off-screen buffer. +// Until SwapInOffscreenRender promotes it, writes go to that buffer and the +// displayed buffer — what every reader sees — is left as it was. This is how an +// async re-render avoids exposing a half-written buffer: it accumulates +// off-screen and swaps in once it has read enough to paint. +func (v *View) BeginOffscreenRender() { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + ei := newEscapeInterpreter(v.outMode) + // The screen width content is wrapped at is render configuration set by + // SetContentWidth, not per-buffer state, so the off-screen buffer's parser + // needs it too — otherwise it counts no soft wraps and cursor-positioning + // escapes land on the wrong rows. + ei.screenColMax = v.buf.ei.screenColMax + v.offscreen = &viewBuffer{ei: ei} +} + +// SwapInOffscreenRender promotes the off-screen buffer (see BeginOffscreenRender) +// to the displayed buffer in one step, so the view jumps straight from the +// previous render to the new one with no half-written frame. Writes after this +// append to the now-displayed buffer directly. It is a no-op if no off-screen +// render is in progress, so it is safe to call more than once (e.g. again at EOF +// after an earlier paint already swapped). +func (v *View) SwapInOffscreenRender() { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + if v.offscreen == nil { + return + } + v.buf = v.offscreen + v.offscreen = nil + v.tainted = true + v.clearHover() +} + +// FreezeScrollbarHeight records the view's current content height so the +// scrollbar keeps that size while a re-render loads, instead of shrinking and +// snapping back as the partially-loaded content streams in past the first paint +// (see scrollbarHeightFloor). Call it when a load begins, while the view still +// shows the previous render; UnfreezeScrollbarHeight clears it when the load +// ends. +func (v *View) FreezeScrollbarHeight() { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + v.refreshViewLinesIfNeeded() + v.scrollbarHeightFloor = len(v.viewLines) +} + +// UnfreezeScrollbarHeight clears the height held by FreezeScrollbarHeight, so +// the scrollbar tracks the view's content directly again. Call it when a load +// ends. +func (v *View) UnfreezeScrollbarHeight() { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + v.scrollbarHeightFloor = 0 +} + +// scrollbarContentHeight is the view-line height the scrollbar is sized from. +// While a re-render is loading it is held at the height the view had when the +// load began (see FreezeScrollbarHeight), so the thumb doesn't shrink and jump +// as partially-loaded content streams in. +func (v *View) scrollbarContentHeight() int { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + v.refreshViewLinesIfNeeded() + return max(len(v.viewLines), v.scrollbarHeightFloor) +} + func (v *View) rewind() { v.buf.ei.reset() v.buf.ei.resetScreenCursor() @@ -1495,6 +1606,13 @@ func (v *View) refreshViewLinesIfNeeded() { } v.firstDirtyLine = len(lines) + // Truncate any entries left over from a previous, longer render. An async + // re-render builds its content off-screen and swaps it in whole (see + // View.offscreen), so the buffer this rebuilds from is always a complete + // render — there is no half-loaded shorter buffer whose tail we'd need to + // keep showing to avoid a flicker, and a leftover tail would just be stale + // lines mapping to the wrong buffer rows. + v.viewLines = v.viewLines[:lineIdx] v.tainted = false } diff --git a/pkg/gocui/view_test.go b/pkg/gocui/view_test.go index 61dc26b52..294f02b32 100644 --- a/pkg/gocui/view_test.go +++ b/pkg/gocui/view_test.go @@ -159,6 +159,102 @@ func TestAutoRenderingHyperlinks(t *testing.T) { assert.Equal(t, "https://example.com", v.buf.lines[0].cells[0].hyperlink) } +// An async re-render builds into an off-screen buffer and swaps it in once it +// has enough to paint, so readers keep seeing the previous render — coherent and +// consistent — until the new content appears in one step. See View.offscreen. +func TestOffscreenRender(t *testing.T) { + v := NewView("name", 0, 0, 80, 10, OutputNormal) + + v.writeString("a\nb\nc") + assert.Equal(t, []string{"a", "b", "c"}, v.ViewBufferLines()) + + // Render new, longer content off-screen. + v.BeginOffscreenRender() + v.writeString("w\nx\ny\nz") + + // The displayed buffer is untouched: readers still see the previous render. + assert.Equal(t, []string{"a", "b", "c"}, v.ViewBufferLines()) + + // Swapping in reveals the new content in one step. + v.SwapInOffscreenRender() + assert.Equal(t, []string{"w", "x", "y", "z"}, v.ViewBufferLines()) + + // A further write now appends to the displayed buffer directly. + v.writeString("\nmore") + assert.Equal(t, []string{"w", "x", "y", "z", "more"}, v.ViewBufferLines()) +} + +// When a render produces fewer view lines than the previous one, +// refreshViewLinesIfNeeded must truncate viewLines to the new content rather +// than leaving the previous render's entries in the tail: with the off-screen +// render there is no half-loaded buffer whose tail we'd want to keep showing, +// and a leftover tail is just stale lines describing content that is gone. +func TestViewLinesTruncatedByShorterRender(t *testing.T) { + v := NewView("name", 0, 0, 10, 10, OutputNormal) // InnerWidth is 9 + v.Wrap = true + + // Two lines of 27 characters each wrap into 3 view lines apiece. + v.writeString(strings.Repeat("a", 27) + "\n" + strings.Repeat("b", 27)) + assert.Equal(t, 6, v.ViewLinesHeight()) + + // Re-render with three short, unwrapped lines: only 3 view lines remain. + v.BeginOffscreenRender() + v.writeString("aaa\nbbb\nccc") + v.SwapInOffscreenRender() + assert.Equal(t, 3, v.ViewLinesHeight()) + assert.Equal(t, []string{"aaa", "bbb", "ccc"}, v.ViewBufferLines()) +} + +// While an async re-render loads, it swaps in only a partially-filled buffer at +// its first paint and keeps appending lines afterwards. The scrollbar must keep +// using the pre-load height until the load ends, so the thumb doesn't shrink and +// snap back as the rest streams in. See View.scrollbarHeightFloor. +func TestScrollbarHeightHeldWhileLoading(t *testing.T) { + v := NewView("name", 0, 0, 80, 12, OutputNormal) + + // Initial render: 100 lines, scrolled well down. + v.writeString(strings.Repeat("x\n", 100)) + v.SetOrigin(0, 80) + assert.Equal(t, 100, v.scrollbarContentHeight()) + + // A re-render begins while the previous render is still shown: hold the + // scrollbar height at the current value. + v.FreezeScrollbarHeight() + + // The off-screen render swaps in only a screenful at its first paint. + v.BeginOffscreenRender() + v.writeString(strings.Repeat("y\n", 30)) + v.SwapInOffscreenRender() + + // The displayed buffer is now short, but the scrollbar height stays held, so + // the thumb keeps its position instead of jumping. + assert.Equal(t, 30, v.ViewLinesHeight()) + assert.Equal(t, 100, v.scrollbarContentHeight()) + + // The rest of the content streams in. + v.writeString(strings.Repeat("y\n", 70)) + assert.Equal(t, 100, v.scrollbarContentHeight()) + + // Once the load ends, the scrollbar tracks the real content directly again. + v.UnfreezeScrollbarHeight() + assert.Equal(t, 100, v.scrollbarContentHeight()) +} + +// If a synchronous render (e.g. a string render) supersedes a still-loading diff +// before it reaches its end, the held scrollbar height must be released, so the +// scrollbar reflects the new content rather than the abandoned load's height. +func TestScrollbarHeightReleasedWhenContentReplaced(t *testing.T) { + v := NewView("name", 0, 0, 80, 12, OutputNormal) + + v.writeString(strings.Repeat("x\n", 100)) + v.FreezeScrollbarHeight() + assert.Equal(t, 100, v.scrollbarContentHeight()) + + // A synchronous render replaces the content before the (notional) load ends. + v.SetContent("just a few\nshort lines\nhere") + assert.Equal(t, 3, v.scrollbarContentHeight()) +} + func TestContainsColoredText(t *testing.T) { hexColor := func(text string, hexStr string) []cell { cells := make([]cell, len(text)) @@ -282,6 +378,31 @@ func TestWriteCursorPositionEscapeAcrossWrites(t *testing.T) { }, got) } +func TestWriteCursorPositionEscapeInOffscreenRender(t *testing.T) { + // Soft-wrap counting has to work in an off-screen render too: the content + // width the parser counts wraps against is set by SetContentWidth before the + // render starts, so the off-screen buffer's parser has to pick it up. If it + // doesn't, no wraps are counted and the CUP below is evaluated against a + // stale row, overshooting into an extra blank line. + v := NewView("name", 0, 0, 30, 30, OutputNormal) + v.SetContentWidth(5) + + v.BeginOffscreenRender() + // Seven characters soft-wrap once on a 5-column screen, putting ConPTY on + // row 2; CUP to row 3 should then skip no rows at all. + v.writeString("aaaaaaa\x1b[3;1Hb\n") + v.SwapInOffscreenRender() + + got := make([][]string, 0, len(v.buf.lines)) + for _, l := range v.buf.lines { + got = append(got, cellsToStrings(l.cells)) + } + assert.Equal(t, [][]string{ + {"a", "a", "a", "a", "a", "a", "a"}, + {"b"}, + }, got) +} + func TestWriteCursorForwardEscape(t *testing.T) { // ConPTY compresses runs of default-colored spaces into ECH (\x1b[NX, // "clear N cells, cursor stationary") + CUF (\x1b[NC, "cursor forward diff --git a/pkg/gui/pty.go b/pkg/gui/pty.go index 1cb8d9412..fb7ba352e 100644 --- a/pkg/gui/pty.go +++ b/pkg/gui/pty.go @@ -77,6 +77,10 @@ func (gui *Gui) newPtyTask(view *gocui.View, cmd *exec.Cmd, prefix string) error // without this the next layout pass would clamp the scroll position to the // not-yet-loaded content. gui.getManager(view).StartLoading() + // Hold the scrollbar at its current height while the re-render loads, so the + // thumb doesn't shrink and snap back when the first partial paint swaps in + // (see the matching call in newCmdTask). + view.FreezeScrollbarHeight() // Run the pty after layout so that it gets the correct size gui.afterLayout(func() error { diff --git a/pkg/gui/tasks_adapter.go b/pkg/gui/tasks_adapter.go index 440bc0aab..f2f567a75 100644 --- a/pkg/gui/tasks_adapter.go +++ b/pkg/gui/tasks_adapter.go @@ -22,6 +22,11 @@ func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error // and before the next layout pass) so the layout doesn't clamp the scroll // position to the not-yet-loaded content. manager.StartLoading() + // Hold the scrollbar at the height the view has now (the previous render), + // while it still shows that render: once the re-render swaps in its first + // partial paint the displayed buffer is briefly short, and we don't want the + // thumb to shrink and snap back as the rest loads. + view.FreezeScrollbarHeight() // Snapshot the view width here, on the UI thread, so the task goroutine // doesn't read the view's live dimensions while it streams output. It's @@ -137,12 +142,10 @@ func (gui *Gui) getManager(view *gocui.View) *tasks.ViewBufferManager { gui.Log, view, func() { - // we could clear here, but that actually has the effect of causing a flicker - // where the view may contain no content momentarily as the gui refreshes. - // Instead, we're rewinding the write pointer so that we will just start - // overwriting the existing content from the top down. Once we've reached - // the end of the content do display, we call view.FlushStaleCells() to - // clear out the remaining content from the previous render. + // Called before showing the "loading..." indicator: clear the + // displayed buffer so only "loading..." is shown. The actual content + // is rendered off-screen (beginRender below) and swapped in, so it + // never overwrites the displayed buffer incrementally. view.Reset() }, func() { @@ -154,6 +157,11 @@ func (gui *Gui) getManager(view *gocui.View) *tasks.ViewBufferManager { gui.renderContentOnly() }, func() { + // The content is fully loaded now, so let the scrollbar track it + // directly again (it was held at the previous render's height while + // loading, see FreezeScrollbarHeight). + view.UnfreezeScrollbarHeight() + // Need to check if the content of the view is well past the origin. linesHeight := view.ViewLinesHeight() _, originY := view.Origin() @@ -168,6 +176,8 @@ func (gui *Gui) getManager(view *gocui.View) *tasks.ViewBufferManager { func() { view.SetOrigin(0, 0) }, + view.BeginOffscreenRender, + view.SwapInOffscreenRender, func() gocui.Task { // A background task: rendering content into a view is display // work, not lazygit driving a git operation, so it must not diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index 0fa04eb3e..918713a05 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -95,6 +95,13 @@ type ViewBufferManager struct { refreshView func() onEndOfInput func() + // beginRender starts an off-screen render: the new content is built without + // disturbing what's displayed. swapInRender then promotes it to the display + // in one step. Together they keep the view showing the previous render until + // the new one has read enough to paint, instead of revealing it line by line. + beginRender func() + swapInRender func() + // see docs/dev/Busy.md // A gocui task is not the same thing as the tasks defined in this file. // A gocui task simply represents the fact that lazygit is busy doing something, @@ -146,6 +153,8 @@ func NewViewBufferManager( refreshView func(), onEndOfInput func(), onNewKey func(), + beginRender func(), + swapInRender func(), newGocuiTask func() gocui.Task, onUIThread func(f func()) error, ) *ViewBufferManager { @@ -156,6 +165,8 @@ func NewViewBufferManager( refreshView: refreshView, onEndOfInput: onEndOfInput, onNewKey: onNewKey, + beginRender: beginRender, + swapInRender: swapInRender, newGocuiTask: newGocuiTask, onUIThread: onUIThread, } @@ -340,8 +351,9 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix // closed the selects below could still service a ready data channel // instead of bailing. Check stop explicitly first to give it priority: // a task that's been stopped (it's being replaced by a newer one) must - // not touch the view here — beforeStart clears it and the prefix gets - // written, clobbering what the incoming task is about to render. + // not touch the view here — it would start an off-screen render and + // write the prefix into it, clobbering what the incoming task is about + // to render. stopped := func() bool { select { case <-opts.Stop: @@ -398,7 +410,10 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix loadingMutex.Lock() if !loaded { - self.beforeStart() + // Build the new content off-screen, leaving the previous render + // displayed until we swap in below; this is what keeps an async + // re-render from showing a half-loaded buffer. + self.beginRender() if prefix != "" { writeToView([]byte(prefix)) } @@ -407,12 +422,16 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix loadingMutex.Unlock() if !ok { - // if we're here then there's nothing left to scan from the source - // so we're at the EOF and can flush the stale content. + // We're at EOF before reaching InitialRefreshAfter (the content was + // shorter than a screenful), so swap in whatever we read now, and + // flush the stale content. // onEndOfInput reads the view's dimensions (to decide // whether to scroll) and sets the origin, both of which // are UI-thread-only, so run it there. - _ = self.onUIThread(self.onEndOfInput) + _ = self.onUIThread(func() { + self.swapInRender() + self.onEndOfInput() + }) // Whatever there was to show is on screen now. self.newContentPending.Store(false) // The content is fully loaded now, so it's safe again for the @@ -447,9 +466,11 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix } if linesRead == linesToRead.InitialRefreshAfter { - // We have read enough lines to fill the view, so do a first refresh - // here to show what we have. Continue reading and refresh again at - // the end to make sure the scrollbar has the right size. + // We have read enough lines to fill the view, so swap the off-screen + // content in and do a first refresh to show it. Continue reading and + // refresh again at the end to make sure the scrollbar has the right + // size. + self.swapInRender() self.newContentPending.Store(false) refreshViewIfStale() } diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index ad4469043..7d10836d8 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -27,6 +27,8 @@ func TestNewCmdTaskInstantStop(t *testing.T) { refreshView, getRefreshViewCallCount := getCounter() onEndOfInput, getOnEndOfInputCallCount := getCounter() onNewKey, getOnNewKeyCallCount := getCounter() + beginRender, getBeginRenderCallCount := getCounter() + swapInRender, getSwapInRenderCallCount := getCounter() onDone, getOnDoneCallCount := getCounter() task := gocui.NewFakeTask() newTask := func() gocui.Task { @@ -40,6 +42,8 @@ func TestNewCmdTaskInstantStop(t *testing.T) { refreshView, onEndOfInput, onNewKey, + beginRender, + swapInRender, newTask, // no UI thread in the test; run the view mutations inline func(f func()) error { f(); return nil }, @@ -69,6 +73,8 @@ func TestNewCmdTaskInstantStop(t *testing.T) { {1, getRefreshViewCallCount(), "refreshView"}, {0, getOnEndOfInputCallCount(), "onEndOfInput"}, {0, getOnNewKeyCallCount(), "onNewKey"}, + {0, getBeginRenderCallCount(), "beginRender"}, + {0, getSwapInRenderCallCount(), "swapInRender"}, {1, getOnDoneCallCount(), "onDone"}, } for _, expectation := range callCountExpectations { @@ -94,6 +100,8 @@ func TestNewCmdTask(t *testing.T) { refreshView, getRefreshViewCallCount := getCounter() onEndOfInput, getOnEndOfInputCallCount := getCounter() onNewKey, getOnNewKeyCallCount := getCounter() + beginRender, getBeginRenderCallCount := getCounter() + swapInRender, getSwapInRenderCallCount := getCounter() onDone, getOnDoneCallCount := getCounter() task := gocui.NewFakeTask() newTask := func() gocui.Task { @@ -107,6 +115,8 @@ func TestNewCmdTask(t *testing.T) { refreshView, onEndOfInput, onNewKey, + beginRender, + swapInRender, newTask, // no UI thread in the test; run the view mutations inline func(f func()) error { f(); return nil }, @@ -136,10 +146,12 @@ func TestNewCmdTask(t *testing.T) { actual int name string }{ - {1, getBeforeStartCallCount(), "beforeStart"}, + {0, getBeforeStartCallCount(), "beforeStart"}, {1, getRefreshViewCallCount(), "refreshView"}, {1, getOnEndOfInputCallCount(), "onEndOfInput"}, {0, getOnNewKeyCallCount(), "onNewKey"}, + {1, getBeginRenderCallCount(), "beginRender"}, + {1, getSwapInRenderCallCount(), "swapInRender"}, {1, getOnDoneCallCount(), "onDone"}, } for _, expectation := range callCountExpectations { @@ -213,6 +225,8 @@ func TestNewCmdTaskQueuedReadAtEndOfInput(t *testing.T) { func() {}, func() {}, func() {}, + func() {}, + func() {}, func() gocui.Task { return task }, // no UI thread in the test; run the view mutations inline func(f func()) error { f(); return nil }, @@ -279,6 +293,8 @@ func TestLoadingIndicatorOnlyTakesOverForNewContent(t *testing.T) { func() {}, func() {}, func() {}, + func() {}, + func() {}, func() gocui.Task { return gocui.NewFakeTask() }, // no UI thread in the test; run the view mutations inline func(f func()) error { f(); return nil }, @@ -392,6 +408,8 @@ func TestNewCmdTaskRefresh(t *testing.T) { refreshView, func() {}, func() {}, + func() {}, + func() {}, newTask, // no UI thread in the test; run the view mutations inline func(f func()) error { f(); return nil }, From bf6c34798ccdde12682954b66e11f052554bcab7 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 8 Aug 2026 22:33:43 +0200 Subject: [PATCH 14/16] Don't run end-of-input handling for a render that was stopped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a task is stopped to make way for a newer one, stopping closes opts.Stop, and the scanner goroutine then closes lineChan. The read loop's select between those two channels is therefore non-deterministic: it can land on the closed lineChan (ok == false) instead of the opts.Stop case, sending a stopped task into the end-of-input branch. There it runs the full finalize — swapping its half-read off-screen buffer in, clamping the origin to the truncated content, and clearing the loading flag — all of which corrupt what the incoming task is about to render. The most visible symptom is a brief frame of truncated content with the scroll yanked to the top, seen when re-renders overlap rapidly (e.g. the periodic background refresh re-rendering a main view faster than it can load, very easy to hit under LAZYGIT_SLOW_RENDER). The underlying bug predates the off-screen render (the EOF branch always clamped the origin via onEndOfInput), but that change made it far worse by also swapping a truncated buffer into the display. Fix it at the source: in the EOF branch, check whether we were stopped and, if so, bail out like the explicit stop case, leaving the view entirely to the task that replaces us. There's no test because the bug is the non-deterministic select itself: any test would have to win a coin flip to observe it. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/tasks/tasks.go | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index 918713a05..6a7e828f4 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -422,9 +422,25 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix loadingMutex.Unlock() if !ok { - // We're at EOF before reaching InitialRefreshAfter (the content was - // shorter than a screenful), so swap in whatever we read now, and - // flush the stale content. + // lineChan is closed. At a genuine end of input we swap in what we + // read and finalize. But lineChan is also closed when this task has + // been stopped to make way for a newer one: stopping closes + // opts.Stop, and the scanner goroutine then closes lineChan, so the + // select above can land here instead of on the opts.Stop case. A + // stopped task is being replaced and must leave the view to the + // incoming task — swapping in its half-read buffer, clamping the + // origin, or clearing `loading` would all corrupt what that task is + // about to render. So bail out here, the same as the explicit stop + // case above. + select { + case <-opts.Stop: + callThen() + break outer + default: + } + // Genuine end of input: swap in whatever we read (the content was + // shorter than a screenful, so we never hit the InitialRefreshAfter + // swap), and flush the stale content. // onEndOfInput reads the view's dimensions (to decide // whether to scroll) and sets the origin, both of which // are UI-thread-only, so run it there. From 3cbf40d4efedc203958b4d2692eaa7275b792c93 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 9 Aug 2026 11:29:25 +0200 Subject: [PATCH 15/16] Reset the scroll to the top at first paint, not when the task starts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a main view re-renders content different from what it last showed, the scroll resets to the top. That reset fired synchronously when the task started — but with the off-screen render the previous content stays displayed until the swap, so resetting the origin up front scrolled that still-visible content to the top before the new content replaced it: a distracting jump when switching commits (or any item) while scrolled down. Defer the reset to the first paint that reveals the new content, so the previous content stays at its scroll until the new content takes its place, and then the new content appears at the top. Swap and reset happen in one hop on the UI thread, so no draw can land between them and show the new content at the old scroll. A same-content re-render keeps its scroll. The "loading..." indicator path also resets the origin now, since it clears the previous content to show the message and must put it at the top. The reset moves out of NewTask into the read loop, keying off the flag that already records whether the render's content is new. NewTask still decides, from the same command-key comparison as before and under the same lock. It has to be that flag rather than per-task state, because a task can be stopped and replaced before it ever paints — a background refresh landing just after the user clicked a different item, which is the ordering a VS Code terminal produces, since it delivers the focus-in event (and so the refresh) before the click. The replacement renders the same content and so sets nothing of its own, and the click's reset would be lost with the task that owed it. The manager's onNewKey callback is renamed resetOrigin to match its now-decoupled timing. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/tasks/tasks.go | 98 +++++++++++++++++++++++-------------- pkg/tasks/tasks_test.go | 104 +++++++++++++++++++++++++++++----------- 2 files changed, 138 insertions(+), 64 deletions(-) diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index 6a7e828f4..17cfabb5f 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -76,13 +76,27 @@ type ViewBufferManager struct { // thread; nil when no task is running. readLines atomic.Pointer[chan LinesToRead] taskKey string - onNewKey func() + + // Resets the view's scroll position to the top. A render whose content is + // different from what the view last showed (a different command key) calls + // this — but at its *first paint*, not when the task starts: the off-screen + // render leaves the previous content displayed until the swap, so resetting + // the origin up front would scroll that still-displayed content to the top + // before the new content replaces it. See newContentPending. + resetOrigin func() // Whether the content the running task is rendering differs from what the - // view is currently showing (i.e. the command key changed). The loading - // indicator only takes the view over when it is set: there is no point - // clearing content we are about to render identically. Cleared once the - // task has rendered enough for the view to be showing the new content. + // view is currently showing (i.e. the command key changed). Two things key + // off it: the loading indicator only takes the view over when it is set, + // since there is no point clearing content we are about to render + // identically; and the first paint that reveals the content resets the + // scroll to the top and clears it. + // + // It deliberately outlives the task that set it: a task can be stopped and + // replaced before it ever paints — a background refresh landing just after + // the user clicked a different item, say — and the replacement, which + // renders the same content and so sets nothing of its own, still has to do + // what that task was owed. newContentPending atomic.Bool // Whether a command task is currently reading content into the view. While @@ -152,7 +166,7 @@ func NewViewBufferManager( beforeStart func(), refreshView func(), onEndOfInput func(), - onNewKey func(), + resetOrigin func(), beginRender func(), swapInRender func(), newGocuiTask func() gocui.Task, @@ -164,7 +178,7 @@ func NewViewBufferManager( beforeStart: beforeStart, refreshView: refreshView, onEndOfInput: onEndOfInput, - onNewKey: onNewKey, + resetOrigin: resetOrigin, beginRender: beginRender, swapInRender: swapInRender, newGocuiTask: newGocuiTask, @@ -324,9 +338,15 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix // it for the message and then rendering the same thing back is a // visible flicker for nothing — and a slow re-render of unchanged // content is common (a background refresh over a repo with submodules - // that have uncommitted changes, say). + // that have uncommitted changes, say). The pending flag isn't consumed + // here; the first paint still owes the scroll reset. if !loaded && self.newContentPending.Load() { self.beforeStart() + // beforeStart cleared the previous content to show "loading...", so + // put the view back at the top for it (beforeStart doesn't touch the + // origin). The origin is view state the UI thread reads while laying + // out, so write it there. + _ = self.onUIThread(self.resetOrigin) _, _ = self.writer.Write([]byte("loading...")) self.refreshView() } @@ -368,6 +388,25 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix // this to work out how many more lines, if any, we still need to read. linesRead := 0 + // The first paint swaps the off-screen render in to reveal the new + // content, and settles the scroll position in the same step — so the new + // content first appears already where it belongs, and no draw can land + // between the two and show it at the previous render's scroll. It happens + // once, either when we've read far enough (below) or at end of input for + // content shorter than that. Callers run it on the UI thread: it writes + // the view's origin. + painted := false + firstPaint := func() { + if painted { + return + } + painted = true + self.swapInRender() + if self.newContentPending.Swap(false) { + self.resetOrigin() + } + } + // Set LAZYGIT_SLOW_RENDER= to sleep that long after each // line is written to the view, stretching async loads out so the frames // of a re-render become visible. Useful for debugging scroll/flicker @@ -438,18 +477,16 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix break outer default: } - // Genuine end of input: swap in whatever we read (the content was - // shorter than a screenful, so we never hit the InitialRefreshAfter - // swap), and flush the stale content. - // onEndOfInput reads the view's dimensions (to decide - // whether to scroll) and sets the origin, both of which - // are UI-thread-only, so run it there. + // Genuine end of input: do the first paint now if it hasn't happened + // yet (the content was shorter than a screenful, so we never reached + // the point below), and flush the stale content. onEndOfInput reads + // the view's dimensions (to decide whether to scroll) and sets the + // origin, both of which are UI-thread-only, so run it there — as is + // firstPaint, which also writes the origin. _ = self.onUIThread(func() { - self.swapInRender() + firstPaint() self.onEndOfInput() }) - // Whatever there was to show is on screen now. - self.newContentPending.Store(false) // The content is fully loaded now, so it's safe again for the // layout to clamp the scroll position to it. We deliberately // don't clear this when stopped (rather than EOF'd), because that @@ -482,12 +519,10 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix } if linesRead == linesToRead.InitialRefreshAfter { - // We have read enough lines to fill the view, so swap the off-screen - // content in and do a first refresh to show it. Continue reading and - // refresh again at the end to make sure the scrollbar has the right - // size. - self.swapInRender() - self.newContentPending.Store(false) + // We have read enough lines to fill the view, so do the first paint + // and refresh to show it. Continue reading and refresh again at the + // end to make sure the scrollbar has the right size. + _ = self.onUIThread(firstPaint) refreshViewIfStale() } } @@ -606,26 +641,19 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error return } + // Note we don't reset the origin here even when the command key changed: + // that's deferred to the first paint that reveals the new content (see + // newContentPending), so the previous content — left displayed until the + // swap — doesn't visibly jump to the top before the new content appears. // Read taskKey directly: we already hold the mutex that guards it, and // GetTaskKey would take it again. - newContent := self.taskKey != key - if newContent { + if self.taskKey != key && self.resetOrigin != nil { self.newContentPending.Store(true) } - resetOrigin := newContent && self.onNewKey != nil self.taskKey = key self.taskIDMutex.Unlock() - if resetOrigin { - // onNewKey resets the view's scroll origin, which is view state the - // UI thread reads while laying out and drawing, so do it there. This - // must happen after releasing taskIDMutex: it blocks until the UI - // thread runs it, and a NewTask call on the UI thread takes - // taskIDMutex, so holding it here would deadlock. - _ = self.onUIThread(self.onNewKey) - } - self.waitingMutex.Lock() // Re-check staleness after acquiring waitingMutex: a newer task diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index 7d10836d8..b15b48ee4 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -26,7 +26,7 @@ func TestNewCmdTaskInstantStop(t *testing.T) { beforeStart, getBeforeStartCallCount := getCounter() refreshView, getRefreshViewCallCount := getCounter() onEndOfInput, getOnEndOfInputCallCount := getCounter() - onNewKey, getOnNewKeyCallCount := getCounter() + resetOrigin, getResetOriginCallCount := getCounter() beginRender, getBeginRenderCallCount := getCounter() swapInRender, getSwapInRenderCallCount := getCounter() onDone, getOnDoneCallCount := getCounter() @@ -41,7 +41,7 @@ func TestNewCmdTaskInstantStop(t *testing.T) { beforeStart, refreshView, onEndOfInput, - onNewKey, + resetOrigin, beginRender, swapInRender, newTask, @@ -72,7 +72,7 @@ func TestNewCmdTaskInstantStop(t *testing.T) { {0, getBeforeStartCallCount(), "beforeStart"}, {1, getRefreshViewCallCount(), "refreshView"}, {0, getOnEndOfInputCallCount(), "onEndOfInput"}, - {0, getOnNewKeyCallCount(), "onNewKey"}, + {0, getResetOriginCallCount(), "resetOrigin"}, {0, getBeginRenderCallCount(), "beginRender"}, {0, getSwapInRenderCallCount(), "swapInRender"}, {1, getOnDoneCallCount(), "onDone"}, @@ -99,7 +99,7 @@ func TestNewCmdTask(t *testing.T) { beforeStart, getBeforeStartCallCount := getCounter() refreshView, getRefreshViewCallCount := getCounter() onEndOfInput, getOnEndOfInputCallCount := getCounter() - onNewKey, getOnNewKeyCallCount := getCounter() + resetOrigin, getResetOriginCallCount := getCounter() beginRender, getBeginRenderCallCount := getCounter() swapInRender, getSwapInRenderCallCount := getCounter() onDone, getOnDoneCallCount := getCounter() @@ -114,7 +114,7 @@ func TestNewCmdTask(t *testing.T) { beforeStart, refreshView, onEndOfInput, - onNewKey, + resetOrigin, beginRender, swapInRender, newTask, @@ -149,7 +149,7 @@ func TestNewCmdTask(t *testing.T) { {0, getBeforeStartCallCount(), "beforeStart"}, {1, getRefreshViewCallCount(), "refreshView"}, {1, getOnEndOfInputCallCount(), "onEndOfInput"}, - {0, getOnNewKeyCallCount(), "onNewKey"}, + {0, getResetOriginCallCount(), "resetOrigin"}, {1, getBeginRenderCallCount(), "beginRender"}, {1, getSwapInRenderCallCount(), "swapInRender"}, {1, getOnDoneCallCount(), "onDone"}, @@ -266,32 +266,78 @@ func TestNewCmdTaskQueuedReadAtEndOfInput(t *testing.T) { assert.True(t, thenCalled) } -// A writer that records whether the loading indicator was ever written to it. -type LoadingIndicatorSpy struct { - sawLoadingIndicator atomic.Bool -} - -func (self *LoadingIndicatorSpy) Write(p []byte) (n int, err error) { - if bytes.Contains(p, []byte("loading...")) { - self.sawLoadingIndicator.Store(true) - } - return len(p), nil -} - -// A render that takes long enough to produce its first line takes the view over -// to say "loading...", which means clearing whatever it was showing. That is only -// worth doing when the content coming is different from what's on screen: -// re-rendering the same content would otherwise clear the view and render the -// same thing straight back, a visible flicker for nothing. -func TestLoadingIndicatorOnlyTakesOverForNewContent(t *testing.T) { - writer := &LoadingIndicatorSpy{} +// A task rendering content the view wasn't already showing resets the scroll +// position to the top, at its first paint. If it is stopped and replaced before +// it ever paints — a background refresh landing just after the user clicked a +// different item, say — the replacement renders the same content and so decides +// on no reset of its own; it has to perform the one the stopped task was owed, +// or the view keeps the scroll position of the content it showed before. +func TestResetOriginSurvivesTaskReplacement(t *testing.T) { + resetOrigin, getResetOriginCallCount := getCounter() manager := NewViewBufferManager( utils.NewDummyLog(), - writer, + bytes.NewBuffer(nil), func() {}, func() {}, func() {}, + resetOrigin, + func() {}, + func() {}, + func() gocui.Task { return gocui.NewFakeTask() }, + // no UI thread in the test; run the view mutations inline + func(f func()) error { f(); return nil }, + ) + + startTask := func(key string, reader io.Reader, onDone func()) { + start := func() (Cmd, io.Reader) { + // not actually starting this because it's not necessary + return ExecCmd{Cmd: exec.Command("blah")}, reader + } + // The first-paint point is far beyond what any of these readers yield, so + // only reaching EOF paints. + _ = manager.NewTask(manager.NewCmdTask(start, "", LinesToRead{100, 50, nil}, onDone), key) + } + runTaskToCompletion := func(key string) { + done := make(chan struct{}) + startTask(key, &BlankLineReader{totalLinesToYield: 3}, func() { close(done) }) + <-done + } + + // A render of content the view wasn't showing resets the scroll position. + runTaskToCompletion("cmd1") + assert.Equal(t, 1, getResetOriginCallCount()) + + // Different content again, but this task stalls before it can paint. + stalled := BlockingLineReader{ + linesToYield: 3, + blocked: make(chan struct{}), + unblock: make(chan struct{}), + } + defer close(stalled.unblock) + startTask("cmd2", &stalled, nil) + <-stalled.blocked + + // The replacement shows the same content as the stalled task, so it has no + // reset of its own to do — but it must still do that task's. + runTaskToCompletion("cmd2") + assert.Equal(t, 2, getResetOriginCallCount()) +} + +// A render that takes long enough to start takes the view over to say +// "loading...", which means blanking whatever it was showing. That is only worth +// doing when the content coming is different from what's on screen: re-rendering +// the same content (a background refresh, say) would otherwise blank the view and +// paint the same thing back, a visible flicker for nothing. +func TestLoadingIndicatorOnlyTakesOverForNewContent(t *testing.T) { + var beforeStartCount atomic.Int32 + + manager := NewViewBufferManager( + utils.NewDummyLog(), + io.Discard, + func() { beforeStartCount.Add(1) }, + func() {}, + func() {}, func() {}, func() {}, func() {}, @@ -325,20 +371,20 @@ func TestLoadingIndicatorOnlyTakesOverForNewContent(t *testing.T) { done := make(chan struct{}) startTask("cmd1", &BlankLineReader{totalLinesToYield: 3}, func() { close(done) }) <-done - assert.False(t, writer.sawLoadingIndicator.Load()) + assert.EqualValues(t, 0, beforeStartCount.Load()) // A slow re-render of that same content must leave the view alone however // long it takes. The indicator is due 200ms in, so give it well past that. sameContent := startStalledTask("cmd1") defer close(sameContent.unblock) time.Sleep(500 * time.Millisecond) - assert.False(t, writer.sawLoadingIndicator.Load()) + assert.EqualValues(t, 0, beforeStartCount.Load()) // Different content, though, is worth taking the view over for. newContent := startStalledTask("cmd2") defer close(newContent.unblock) assert.Eventually(t, - writer.sawLoadingIndicator.Load, + func() bool { return beforeStartCount.Load() == 1 }, 2*time.Second, 10*time.Millisecond) } From ebfa8c71b2274e75766b671ab39d5050768228a0 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 9 Aug 2026 08:15:36 +0200 Subject: [PATCH 16/16] Drop FlushStaleCells, which no longer has anything to flush It existed for the incremental re-render: a shorter render left the previous one's view lines in the tail (deliberately, to avoid a blank frame), and this cleared them once the new content was fully read. Async renders now build off-screen and swap in whole, so refreshViewLinesIfNeeded truncates the view lines to the buffer and no tail can form. All the call at end-of-input still did was discard every wrapped line and force the whole buffer to be re-wrapped on the next draw, which is pure work on a large diff. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/commands/oscommands/pty_windows.go | 3 ++- pkg/gocui/view.go | 11 ----------- pkg/gui/tasks_adapter.go | 2 -- 3 files changed, 2 insertions(+), 14 deletions(-) diff --git a/pkg/commands/oscommands/pty_windows.go b/pkg/commands/oscommands/pty_windows.go index e645ae627..ff707c519 100644 --- a/pkg/commands/oscommands/pty_windows.go +++ b/pkg/commands/oscommands/pty_windows.go @@ -204,7 +204,8 @@ func (p *winPty) Close() error { // slave closes on child exit, but ConPTY keeps the pipe alive until we call // ClosePseudoConsole explicitly. Without doing that on child exit, the // scanner in pkg/tasks.NewCmdTask would block forever on the next read and -// the post-content view never gets cleared (FlushStaleCells never fires). +// the render would never reach its end of input, so the new content would +// never be swapped in. func startWaiter(proc *os.Process, p *winPty) func() error { done := make(chan struct{}) var waitErr error diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index fe01e50ca..dba71ab52 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -1248,17 +1248,6 @@ func (v *View) Reset() { v.offscreen = nil } -// This is for when we've done a restart for the sake of avoiding a flicker and -// we've reached the end of the new content to display: we need to clear the remaining -// content from the previous round. We do this by setting v.viewLines to nil so that -// we just render the new content from v.buf.lines directly -func (v *View) FlushStaleCells() { - v.writeMutex.Lock() - defer v.writeMutex.Unlock() - - v.clearViewLines() -} - // BeginOffscreenRender starts building a re-render into an off-screen buffer. // Until SwapInOffscreenRender promotes it, writes go to that buffer and the // displayed buffer — what every reader sees — is left as it was. This is how an diff --git a/pkg/gui/tasks_adapter.go b/pkg/gui/tasks_adapter.go index f2f567a75..5e5295639 100644 --- a/pkg/gui/tasks_adapter.go +++ b/pkg/gui/tasks_adapter.go @@ -170,8 +170,6 @@ func (gui *Gui) getManager(view *gocui.View) *tasks.ViewBufferManager { view.SetOrigin(0, newOriginY) } - - view.FlushStaleCells() }, func() { view.SetOrigin(0, 0)