Let a re-render put the view back where it was

A view that re-renders content the user is already looking at, laid out
differently — a different context size, another diff renderer — starts the
new render from the top, losing where they were. Where that is can't be
carried over as a scroll position, because a different layout of the same
content puts the line they were on somewhere else; it can only be found by
looking at what the re-render brings in.

So let a restore be installed on the buffer manager just before the
re-render is triggered. It rides the next command task, which asks it after
each line whether enough has arrived to show what it remembers, and then
hands it the first paint: the restore searches the off-screen buffer, swaps
it in, and places the view, in that order, so that the search happens while
the previous content is still displayed and the new content is never drawn
at the previous render's scroll position. A restore that placed the view
keeps the scroll reset new content would otherwise get; one that couldn't
find what it was looking for leaves the render to do what it would have
done anyway, and the lines-read count still has the last word on when to
paint, so a restore can never hold a render back for ever.

Some renderings can't be searched at all: a diff renderer is free to say
nothing about which line of which file each row shows, and then no line of
the old rendering can be looked for in the new one. There the offset into
the content is all that is left to go on, and it is nearer to where the user
was than the top is, so a re-render can also ask merely to be left where it
is. That request rides the next task the same way, and answers the same
question the scroll reset and the loading placeholder are asking: whether
what is coming is content the user has not seen.

Both outlive the task they were installed for, like the pending scroll reset
does and for the same reason: that task can be stopped and replaced by a
background refresh before it ever paints, leaving the replacement to honour
it. The loading placeholder stays out of the way while either is pending —
blanking the view for a message before putting the user back where they were
is the flicker they exist to avoid.

Nothing installs either of them yet.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

pkg/tasks/tasks.go

@@ -98,6 +98,24 @@ type ViewBufferManager struct {
 	// what that task was owed.
 	newContentPending atomic.Bool
 
+	// When set, the next command task puts the view back where it was once it has
+	// re-rendered the content, instead of showing the new render from the top (see
+	// RenderRestore). It is installed just before the re-render is triggered.
+	//
+	// Like newContentPending it outlives the task it was installed for, and for the
+	// same reason: that task can be stopped and replaced before it ever paints, and
+	// the replacement, rendering the same content, is then the one that owes the
+	// user their position. It is cleared by whichever task applies it. Guarded by
+	// taskIDMutex, like the task key.
+	restoreForNextTask *RenderRestore
+
+	// When set, the next command task leaves the view's scroll position alone even
+	// though it renders a different command's output, that output being the same
+	// content laid out differently (see SetKeepScrollPositionForNextTask). The task
+	// that starts consumes it, in place of noting that new content is on its way.
+	// Guarded by taskIDMutex, like the task key.
+	keepScrollForNextTask 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.
@@ -152,6 +170,80 @@ type LinesToRead struct {
 	Then func()
 }
 
+// RenderRestore puts a view back where it was when it re-renders content the user
+// is already looking at, laid out differently — a different context size, whitespace
+// ignored, another diff renderer — instead of showing the new render from the top.
+//
+// The task reads the new content into an off-screen buffer; the restore says when
+// enough of it has arrived to show the remembered position (FirstPaintReady), and
+// then finds that position and reveals it (Apply). It is a pair of callbacks rather
+// than a scroll position because a different layout of the same content puts the
+// remembered line somewhere else, and only the new content itself says where.
+type RenderRestore struct {
+	// FirstPaintReady reports whether enough of the new content has been read for
+	// the restore to show what it is looking for. It is consulted after each line
+	// is read, on the task's own goroutine.
+	FirstPaintReady func() bool
+
+	// Apply runs once, on the UI thread, at the first paint. It finds its target in
+	// the off-screen content, calls swapIn to promote that content to the display,
+	// and places the view on the target — in that order, so that the search runs
+	// while the previous content is still displayed, and the new content is never
+	// drawn at the previous render's scroll position.
+	//
+	// It must call swapIn either way, and reports whether it placed the view: when
+	// it didn't, because what it was looking for is not in the new content, the
+	// task does what it would have done without a restore.
+	Apply func(swapIn func()) bool
+}
+
+// SetRestoreForNextTask arranges for the next command task to put the view back
+// where it is now once it has re-rendered. Call it right before triggering a
+// re-render of the content the view is showing; see RenderRestore.
+func (self *ViewBufferManager) SetRestoreForNextTask(restore *RenderRestore) {
+	self.taskIDMutex.Lock()
+	defer self.taskIDMutex.Unlock()
+
+	self.restoreForNextTask = restore
+}
+
+func (self *ViewBufferManager) getRestoreForNextTask() *RenderRestore {
+	self.taskIDMutex.Lock()
+	defer self.taskIDMutex.Unlock()
+
+	return self.restoreForNextTask
+}
+
+// SetKeepScrollPositionForNextTask arranges for the next command task to leave the
+// view's scroll position alone, rather than showing its content from the top the way a
+// render of different content does. Call it right before triggering a re-render of the
+// content the view is showing, when the command producing it is not the one that
+// produced what is on screen — a different context size, another diff renderer.
+//
+// It is the coarser sibling of SetRestoreForNextTask, for the same moment. The restore
+// puts the view back on the line it remembers, which it can only do when the lines of
+// the new rendering can be told apart. This one says merely "the content is a
+// rearrangement of what is there, so the offset into it is nearer to where the user was
+// than the top is". Both can be set at once, and then the restore has the first say.
+func (self *ViewBufferManager) SetKeepScrollPositionForNextTask() {
+	self.taskIDMutex.Lock()
+	defer self.taskIDMutex.Unlock()
+
+	self.keepScrollForNextTask = true
+}
+
+// clearRestore drops a restore once a task has applied it, so that it rides exactly
+// one re-render. One installed since — the user pressing the key again while this
+// task was still reading — is left alone: it belongs to the render on its way.
+func (self *ViewBufferManager) clearRestore(restore *RenderRestore) {
+	self.taskIDMutex.Lock()
+	defer self.taskIDMutex.Unlock()
+
+	if self.restoreForNextTask == restore {
+		self.restoreForNextTask = nil
+	}
+}
+
 func (self *ViewBufferManager) GetTaskKey() string {
 	self.taskIDMutex.Lock()
 	defer self.taskIDMutex.Unlock()
@@ -272,6 +364,10 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
 			onFirstPageShown()
 		}
 
+		// Whatever position is owed to the user belongs to this render: it was
+		// remembered just before the re-render that led here was triggered.
+		restore := self.getRestoreForNextTask()
+
 		if self.throttle.Load() {
 			self.Log.Info("throttling task")
 			time.Sleep(THROTTLE_TIME)
@@ -370,7 +466,12 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
 				// content is common (a background refresh over a repo with submodules
 				// 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() {
+				//
+				// A restore keeps the view too: it is there to make a re-render of what
+				// the user is looking at seamless, and blanking the view for a message
+				// before putting them back where they were is the flicker it exists to
+				// avoid.
+				if !loaded && restore == nil && 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
@@ -431,6 +532,18 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
 					return
 				}
 				painted = true
+				if restore != nil {
+					// The restore does the swap itself, so that it can find where the
+					// user was in the new content before it is revealed.
+					placed := restore.Apply(self.swapInRender)
+					self.clearRestore(restore)
+					if placed {
+						// The view is where the user left it, which is exactly what the
+						// scroll reset would undo.
+						self.newContentPending.Store(false)
+						return
+					}
+				}
 				self.swapInRender()
 				if self.newContentPending.Swap(false) {
 					self.resetOrigin()
@@ -469,7 +582,13 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
 							linesToRead.Then()
 						}
 					}
-					for linesToRead.Total == -1 || linesRead < linesToRead.Total {
+					// A restore that hasn't painted yet keeps us reading past the lines
+					// asked for, all the way to the end of the input if need be. What it
+					// is looking for may be anywhere in the new content, and a rendering
+					// that has to be parsed as a diff to be searched at all can only be
+					// parsed whole — so stopping early would leave it nothing to find,
+					// and the view somewhere the user didn't put it.
+					for linesToRead.Total == -1 || linesRead < linesToRead.Total || (restore != nil && !painted) {
 						if stopped() {
 							callThen()
 							break outer
@@ -540,12 +659,22 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
 							time.Sleep(slowRenderPerLine)
 						}
 
-						if linesRead == linesToRead.InitialRefreshAfter {
-							// 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
+						if !painted {
+							// Do the first paint once we have read enough lines to fill the
+							// view — or, when a position is waiting to be restored, once the
+							// restore says it can show it, since where the view should be is
+							// its call. Continue reading afterwards and refresh again at the
 							// end to make sure the scrollbar has the right size.
-							_ = self.onUIThread(firstPaint)
-							refreshViewIfStale()
+							var ready bool
+							if restore != nil {
+								ready = restore.FirstPaintReady()
+							} else {
+								ready = linesRead == linesToRead.InitialRefreshAfter
+							}
+							if ready {
+								_ = self.onUIThread(firstPaint)
+								refreshViewIfStale()
+							}
 						}
 					}
 					refreshViewIfStale()
@@ -672,10 +801,19 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error
 		// 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.
-		if self.taskKey != key && self.resetOrigin != nil {
+		// GetTaskKey would take it again. A pending restore isn't dropped here
+		// either, even for a different command: the re-renders it rides are all
+		// different commands (a different context size, another diff renderer), and
+		// it validates itself against the content it lands in anyway.
+		// A task told to keep the scroll position renders the content the view is
+		// already showing, laid out differently, so the reset it would otherwise owe
+		// would take the user away from what they are reading — and the loading
+		// message, which the same flag governs, would blank content that is about to
+		// come back looking much the same.
+		if self.taskKey != key && self.resetOrigin != nil && !self.keepScrollForNextTask {
 			self.newContentPending.Store(true)
 		}
+		self.keepScrollForNextTask = false
 		self.taskKey = key
 
 		self.taskIDMutex.Unlock()

pkg/tasks/tasks_test.go

@@ -385,6 +385,231 @@ func TestLoadingIndicatorOnlyTakesOverForNewContent(t *testing.T) {
 		2*time.Second, 10*time.Millisecond)
 }
 
+// A pending restore takes the first paint over: it says when enough of the new
+// content has arrived to show the position it remembers, and does the swap itself so
+// that it can look for that position while the previous content is still displayed.
+// Having put the view where the user left it, it also keeps the scroll reset that new
+// content would otherwise get.
+func TestNewCmdTaskRestore(t *testing.T) {
+	writer := bytes.NewBuffer(nil)
+	linesWritten := func() int { return strings.Count(writer.String(), "\n") }
+	resetOrigin, getResetOriginCallCount := getCounter()
+
+	swapped := false
+	applyCount := 0
+	applyAtLines := -1
+	swappedBeforeApply := false
+	swappedByApply := false
+
+	manager := NewViewBufferManager(
+		utils.NewDummyLog(),
+		writer,
+		func() {}, // beforeStart
+		func() {}, // refreshView
+		func() {}, // onEndOfInput
+		resetOrigin,
+		func() {},                 // beginRender
+		func() { swapped = true }, // swapInRender
+		func() gocui.Task { return gocui.NewFakeTask() },
+		// no UI thread in the test; run the view mutations inline
+		func(f func()) error { f(); return nil },
+	)
+
+	manager.SetRestoreForNextTask(&RenderRestore{
+		// Ready once five lines have loaded — well before the view is filled (30).
+		FirstPaintReady: func() bool { return linesWritten() >= 5 },
+		Apply: func(swapIn func()) bool {
+			applyCount++
+			applyAtLines = linesWritten()
+			swappedBeforeApply = swappedBeforeApply || swapped
+			swapIn()
+			swappedByApply = swapped
+			return true
+		},
+	})
+
+	done := make(chan struct{})
+	start := func() (Cmd, io.Reader) {
+		// not actually starting this because it's not necessary
+		return ExecCmd{Cmd: exec.Command("blah")}, &BlankLineReader{totalLinesToYield: 50}
+	}
+	_ = manager.NewTask(manager.NewCmdTask(start, "", LinesToRead{100, 30, nil}, func() { close(done) }), "cmd")
+	<-done
+
+	assert.Equal(t, 1, applyCount, "Apply should run exactly once")
+	assert.False(t, swappedBeforeApply, "the off-screen render should not be swapped in before Apply runs")
+	assert.True(t, swappedByApply, "Apply should swap the off-screen render in via swapIn")
+	// The first paint was driven by the restore, not by having read enough lines to
+	// fill the view.
+	assert.GreaterOrEqual(t, applyAtLines, 5)
+	assert.Less(t, applyAtLines, 30)
+	assert.Equal(t, 0, getResetOriginCallCount(), "a restore that placed the view leaves the scroll alone")
+}
+
+// A restore that never finds what it is looking for keeps the task reading to the
+// end of its input, since the line might have been anywhere in it. Once there is no
+// more content to hope for, the render is revealed with the scroll reset that new
+// content is owed.
+func TestNewCmdTaskRestoreThatFindsNothing(t *testing.T) {
+	writer := bytes.NewBuffer(nil)
+	linesWritten := func() int { return strings.Count(writer.String(), "\n") }
+	resetOrigin, getResetOriginCallCount := getCounter()
+
+	applyCount := 0
+	swappedAtLines := -1
+
+	manager := NewViewBufferManager(
+		utils.NewDummyLog(),
+		writer,
+		func() {}, // beforeStart
+		func() {}, // refreshView
+		func() {}, // onEndOfInput
+		resetOrigin,
+		func() {}, // beginRender
+		func() { swappedAtLines = linesWritten() },
+		func() gocui.Task { return gocui.NewFakeTask() },
+		// no UI thread in the test; run the view mutations inline
+		func(f func()) error { f(); return nil },
+	)
+
+	manager.SetRestoreForNextTask(&RenderRestore{
+		FirstPaintReady: func() bool { return false },
+		Apply: func(swapIn func()) bool {
+			applyCount++
+			swapIn()
+			return false
+		},
+	})
+
+	done := make(chan struct{})
+	start := func() (Cmd, io.Reader) {
+		// not actually starting this because it's not necessary
+		return ExecCmd{Cmd: exec.Command("blah")}, &BlankLineReader{totalLinesToYield: 50}
+	}
+	_ = manager.NewTask(manager.NewCmdTask(start, "", LinesToRead{100, 30, nil}, func() { close(done) }), "cmd")
+	<-done
+
+	assert.Equal(t, 1, applyCount, "Apply should still run, to swap the render in")
+	assert.Equal(t, 50, swappedAtLines, "the whole input should be read before giving up on the restore")
+	assert.Equal(t, 1, getResetOriginCallCount(), "new content the restore couldn't place starts at the top")
+}
+
+// The task a restore was installed for can be stopped and replaced before it ever
+// paints — a background refresh landing right after the key was pressed. The
+// replacement renders the same content, so it is the one that owes the user their
+// position.
+func TestRestoreSurvivesTaskReplacement(t *testing.T) {
+	var applyCount atomic.Int32
+
+	manager := NewViewBufferManager(
+		utils.NewDummyLog(),
+		io.Discard,
+		func() {},
+		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 },
+	)
+
+	manager.SetRestoreForNextTask(&RenderRestore{
+		FirstPaintReady: func() bool { return false },
+		Apply: func(swapIn func()) bool {
+			applyCount.Add(1)
+			swapIn()
+			return true
+		},
+	})
+
+	startTask := func(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), "cmd")
+	}
+
+	// The task the restore was installed for stalls before it can paint.
+	stalled := BlockingLineReader{
+		linesToYield: 3,
+		blocked:      make(chan struct{}),
+		unblock:      make(chan struct{}),
+	}
+	defer close(stalled.unblock)
+	startTask(&stalled, nil)
+	<-stalled.blocked
+
+	done := make(chan struct{})
+	startTask(&BlankLineReader{totalLinesToYield: 3}, func() { close(done) })
+	<-done
+
+	assert.EqualValues(t, 1, applyCount.Load(), "the replacement should apply the restore the stopped task couldn't")
+}
+
+// A task told to keep the scroll position renders the content the view is showing
+// under another command — the same diff with more context around it, say — so it
+// neither resets the scroll nor blanks the view to say "loading...", both of which are
+// for content the user hasn't seen.
+func TestKeepScrollPositionForNextTask(t *testing.T) {
+	var beforeStartCount atomic.Int32
+	resetOrigin, getResetOriginCallCount := getCounter()
+
+	manager := NewViewBufferManager(
+		utils.NewDummyLog(),
+		io.Discard,
+		func() { beforeStartCount.Add(1) },
+		func() {}, // refreshView
+		func() {}, // onEndOfInput
+		resetOrigin,
+		func() {}, // beginRender
+		func() {}, // swapInRender
+		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)
+	}
+	runTaskToCompletion := func(key string) {
+		done := make(chan struct{})
+		startTask(key, &BlankLineReader{totalLinesToYield: 3}, func() { close(done) })
+		<-done
+	}
+
+	// Content the view wasn't showing, to have something to keep the position in.
+	runTaskToCompletion("cmd1")
+	assert.Equal(t, 1, getResetOriginCallCount())
+
+	manager.SetKeepScrollPositionForNextTask()
+	runTaskToCompletion("cmd2")
+	assert.Equal(t, 1, getResetOriginCallCount(), "the same content under another command keeps its position")
+
+	// And the request rides one task only: the next different command is a different
+	// diff as far as anyone knows.
+	runTaskToCompletion("cmd3")
+	assert.Equal(t, 2, getResetOriginCallCount())
+
+	// The loading indicator goes by the same question, so it stays out of the way too.
+	manager.SetKeepScrollPositionForNextTask()
+	stalled := BlockingLineReader{
+		blocked: make(chan struct{}),
+		unblock: make(chan struct{}),
+	}
+	defer close(stalled.unblock)
+	startTask("cmd4", &stalled, nil)
+	<-stalled.blocked
+	time.Sleep(500 * time.Millisecond)
+	assert.EqualValues(t, 0, beforeStartCount.Load())
+}
+
 func TestNewCmdTaskRefresh(t *testing.T) {
 	type scenario struct {
 		name                        string