Fix flicker, scroll glitches, and crashes in async diff rendering - #5938
Merged
Conversation
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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=<milliseconds> 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…ging 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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>
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) <noreply@anthropic.com>
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a preparation PR for the upcoming fold-staging-into-main-view work; see the individual commit messages for details.
The most notable change is probably that we switch to a double-buffering approach for flicker-free view updates; previously we would overwrite the view from the top, and keep the existing viewlines below untouched to update without flicker. This caused numerous problems though that will become more painful when we start using the main view for more operations (especially staging); telling whether the selected line still belongs to the previous task or already to the new one is tricky. Rendering into an offscreen buffer and swapping it in as soon as we have enough to fill the screen makes this much easier.