utils-code/jesseduffield.lazygit
1
0
Fork 0
mirror of https://github.com/jesseduffield/lazygit.git synced 2026-09-10 07:36:27 -04:00
Code Issues Actions 3 Packages Projects Releases Wiki Activity

Reset the scroll to the top at first paint, not when the task starts

Browse source 
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) <noreply@anthropic.com>
This commit is contained in:
Stefan Haller 2026-08-09 11:29:25 +02:00
parent bf6c34798c
commit 3cbf40d4ef
2 changed files with 138 additions and 64 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

98
pkg/tasks/tasks.go
View file

@ -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=<milliseconds> 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

104
pkg/tasks/tasks_test.go
View file

@ -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)
}