From 3cbf40d4efedc203958b4d2692eaa7275b792c93 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 9 Aug 2026 11:29:25 +0200 Subject: [PATCH] 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) }