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, the diff-line
mapping, clicks) could observe a half-written buffer at the wrong scroll —
the §11 Race A flicker and the §8 stale-tail mapping both came from this.
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 and applies the
saved scroll in the same step, 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 the stale
tail (§8) never forms. clear()/Reset() abandon any in-progress off-screen
render so a synchronous SetContent after a stopped task writes to the
display.
This replaces the holdViewLines and freshViewLineCount patches reverted in
the previous two commits with one mechanism. 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 pager).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This reverts the freshViewLineCount stale-tail guard (§8), restoring the
demonstrate-the-bug test state. The upcoming off-screen render rebuilds
the displayed buffer wholesale on swap and lets refreshViewLinesIfNeeded
truncate, so the stale tail never forms — a cleaner fix than tracking a
fresh-count. Removing the guard on its own keeps that change focused; the
bug it guarded against is re-fixed by the truncation in the next commit.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This reverts the holdViewLines flicker patch (Race A). It is about to be
superseded by rendering a re-render into an off-screen buffer and swapping
it in atomically, which keeps the displayed buffer (and so every reader)
untouched until the new content is ready — a cleaner mechanism than
suppressing the view-line rebuild. Removing it on its own keeps that
upcoming change focused.
Co-Authored-By: Claude Opus 4.8 (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 4.8 (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 4.8 (1M context) <noreply@anthropic.com>
When escaping to a focused main view scrolled down, the re-render keeps
the previous content as a placeholder and the task scrolls to the saved
position at its first paint. But the task's writes mark the view tainted,
so any layout pass landing between the first write and the first paint
rebuilds the view lines from the half-loaded buffer and draws them at the
placeholder's scroll — a brief frame of the wrong content at the wrong
scroll before it snaps into place. Intermittent, and only visible when the
load is slow enough for a layout pass to fall in that window.
Give the view a hold: while set, refreshViewLinesIfNeeded keeps the
current view lines instead of rebuilding from the buffer, so the view goes
on drawing the coherent placeholder. The re-render task sets it only when
restoring a scroll position (so the normal load-from-top case is
untouched) and releases it at its first paint, which applies the saved
scroll in the same step — so the loaded content appears at the restored
scroll with no intermediate frame. While held, the displayed view lines
need not match the loading buffer, so the view-line→buffer-line mapping
reports no result.
This needs interactive verification (LAZYGIT_SLOW_RENDER + a real pager);
see focused-main-view-notes.md §13.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Escaping to a focused main view restored the selection by scheduling, on
the next UI tick, a ReadToEnd whose callback re-selected the saved line.
But ReadToEnd fires its callback synchronously when the manager has no
live read channel, and the re-render task triggered by the push creates
that channel later, inside its own goroutine (after stopping the previous
task). If the UI tick won that race, the restore ran before any content
was loaded, FocusPoint no-oped against the unloaded line, and the
selection was silently dropped — intermittently, and more often under
load.
Thread the restore through the task instead: a thenForNextTask hook on
the buffer manager, folded into the next cmd/pty task's initial-read Then,
mirroring scrollToOriginYForNextTask. It runs once the task has read
enough to place the selection, and can't fire before the task exists. The
scroll restore already applies at the task's first paint, which precedes
the initial read's end, so the origin is in place when the selection is
restored.
This needs interactive verification (LAZYGIT_SLOW_RENDER + a real pager);
see focused-main-view-notes.md §13.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pin the two escape-restore races to concrete mechanisms: Race A (layout
draws reveal partial content at the placeholder scroll before the task's
first paint) and Race B (the selection restore can fire before the
re-render task's readLines channel is live). Classify both as bounded
interleavings, sitting over the one fundamental constraint (no flicker-free
first paint of the target scroll without buffering that screenful), and
record the prescribed fixes and their implications for part 3. Documents
why faithful repro is the interactive app, not the headless harness.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Track how many leading viewLines entries the most recent refresh built
from the current buffer (freshViewLineCount) and bound the view-line→
buffer-line mapping on it. The entries past that count are the stale tail
refreshViewLinesIfNeeded leaves in place for flicker-avoidance; they
belong to a previous, longer render and must not be mapped. The old
in-range guard was insufficient: with wrapping a stale entry's buffer
index can still be in range of the shrunk buffer. Within the fresh range
every entry was just built from the current buffer, so its index is
guaranteed in range and the guard is no longer needed.
This is the §8 correctness fix the identity-based escape restore depends
on, since that read scans the view buffer while it is still loading.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
refreshViewLinesIfNeeded keeps the previous render's view lines in the
tail when the new render is shorter, so the view can go on showing old
content without flicker until the new content catches up. The mapping
readers guard only against a buffer index that has gone out of range of
the shrunk buffer — but with wrapping, a stale tail entry's index can
still be in range, so the guard passes and a view line that no longer
exists is mapped onto the wrong buffer line. See diff-line-metadata-notes.md §8.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
HyperLinkInLine, DiffLineMetadataInLine and BufferLineForViewLine each
repeated the same preamble: take the lock, refresh the view lines, range-
check the view line, and guard against a stale viewLines entry pointing
past a shrunk buffer. They all need that mapping to stay consistent with
the buffer they then read, so the logic belongs in one place. Extract it
into bufferLineForViewLine and have all three call it. Behavior-preserving.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Capture a design discussion (no code yet; implementation is a future session):
- The escape-from-staging restore should anchor on the explorer view's current
patch identity at escape time, not a saved numeric scroll/index, since staging
or dropping hunks changes the content. This is the inverse direction of the
diff-line-metadata primitive (identity -> rendered row) and the same operation
the -U scroll-preservation consumer needs; record it as consumer #6 and split
the consumer list into forward (1-4) and inverse (5-6) directions.
- Record the escape-routing cases the current prototype gets wrong (staging the
last hunk should land in the staged half; <tab> between staged/unstaged; the
empty-view and custom-patch-builder cases).
- Decide to solve the new restore mechanism, the §11 timing races, and the
BufferLineForViewLine staleness trap together in the prototype rather than
defer them to productionization (you can't plan around unsolved entangled
mechanisms), with a dependency-first attack order (§8 fix, characterize the
races, then the predicate-scroll restore).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The emitter (delta), carrier (gocui per-cell attachment), and consumer (the
GetDiffLineInfo metadata backend + env-var handshake) are now built for the
normal unified case and verified end-to-end — including in the running app with
delta's default mode, where clicking/enter/e/G resolve via #2 and deletions get
the correct side. Update §9 from "in progress" to a "built & verified" record
(what was built, how it was verified) mirroring §8 for #1, and mark the build
order (§7) accordingly. The remaining step-5 deliverables — finalizing/publishing
the spec and the production plan — and side-by-side/difftastic are still open.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add mechanism #2 to GetDiffLineInfo: when a patched pager annotated each diff
line with OSC 456 metadata, read it back as the first backend, ahead of the
buffer parser (#1) and the lazygit-edit hyperlink. It is strictly higher
fidelity -- it carries the side explicitly, so it serves the renderings #1
cannot parse (delta's default mode, --line-numbers, diff-so-fancy) and conveys
deletions, which the hyperlink can't.
The host advertises the protocol versions it understands by setting
EMIT_OSC456_METADATA on the pager subprocess; a pager that doesn't understand
it ignores the variable, so this is safe to set unconditionally.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A patched pager (delta) prefixes each diff line with an OSC 456 sequence
carrying that line's patch-space identity (see diff-line-metadata-notes.md),
so the host can map a rendered row back to (file, type, new-line, old-line)
without re-parsing -- the only way to recover the side for renderings that
drop the +/- markers (delta's default mode).
Recognize it in the escape interpreter and stamp the payload onto each cell,
mirroring how OSC-8 hyperlinks are handled, exposing it via
DiffLineMetadataInLine. To do so, generalize the OSC dispatch to accumulate
the (possibly multi-digit) OSC number before branching, rather than matching
the single character '8'; the OSC-8 path is unchanged for well-formed input.
Unlike a hyperlink, the metadata sequence is never closed -- the pager
re-emits one per line -- so clear it at each line boundary to keep it from
bleeding onto a following line that has none (e.g. a hunk header).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mechanism #2's emitter side is now prototyped on this branch (delta, normal
unified mode only) and the bytes are verified. Capture what that settled, since
the spec is meant to be published for pager-developer feedback:
- The single per-line emit point in delta and which fields are reachable there
(the gotcha: delta only maintains its line-number counters with --line-numbers
on, so the patch tracks its own), and why a dedicated additive emitter beats
reusing LineNumbersData.
- The pinned v1 wire format (positional, file last so it may contain ';',
empty old-line unless deleted) and the EMIT_OSC456_METADATA env-var handshake.
- Deferred items: the OSC-number terminal audit, wrapped continuation rows, and
header-row attachments.
Also resolves the §6 wire-format open question accordingly.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Record what the #1 prototype settled. The two open questions #1 touches are now
answered (the deleted-line new-line convention is patch.LineNumberOfLine's, and a
deletion carries both line numbers; multi-file diffs split on "diff --git" and
the section parses 1:1 with patch.Parse). Add a §8 capturing what landed, how it
was verified, and the implications for #2.
Two coverage corrections came out of verifying against real pager output, both
worth pinning before the spec is written: delta --color-only qualifies for #1
only *without* line numbers (the gutter pushes the +/- marker off column 0, so a
naive parse is confidently wrong — handled by an integrity check that falls back,
not by teaching the host delta's gutter), and diff-so-fancy strips the +/- markers
entirely, so it's a #2 case, not #1.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The focused main view's click/enter/e/G handlers all need the same thing: given
a rendered diff row, the patch-space line it corresponds to. Until now that came
solely from delta's lazygit-edit:// hyperlinks, which only carry a path and a
single line number — no side. That's lossy: for a deletion the number is the old
line, but the consumers fed it into new-file lookups, and two consecutive
deletions (which share a new-file line number) couldn't be told apart at all.
Replace GetFileAndLineForClickedDiffLine with GetDiffLineInfo, returning the
fuller (file, type, new-line, old-line) record from diff-line-metadata-notes.md.
This is mechanism #1: parse the decolorized view buffer — walk up to the file's
"diff --git" section, reuse patch.Parse on it (splitting multi-file commit diffs
on the "diff --git" boundaries), and read the type and line numbers off the patch
arithmetic. It serves the structure-preserving renderings — no pager, git diff
--color, and delta --color-only without line numbers — with no external
dependency.
To avoid trusting a mis-parse, the parser bails when a hunk's body no longer
matches its header (Patch.IsWellFormed). That's what happens when a pager keeps
the diff/hunk headers but restructures the body: delta's line-number gutters push
the +/- marker off the start of each line, so every body line reads as context.
Such renderings fall through to the next backend rather than yielding a confident
wrong answer. (diff-so-fancy goes further and rewrites the headers too, so it
fails even earlier, on the missing "diff --git".)
GetDiffLineInfo is a seam with swappable backends: the buffer parser first, then
the old hyperlink reader as a fallback for renderings the parser can't handle
(delta's default mode, or delta with line-number gutters). The future #2 OSC
per-cell metadata reader plugs in ahead of both, behind the same record shape.
Wire the consumers to the record per that doc's field mapping:
- dive into staging/patch building lands on the exact patch line, looking a
deletion up by its old-file line number (PatchLineForOldLineNumber) so the
two-deletions case resolves correctly;
- `e` edits at the new-file line;
- `G` anchors the PR link on the left (old) side for a deletion, the right (new)
side otherwise.
The hyperlink fallback can't convey the side, so it reports DiffLineOther, which
the consumers treat as a non-deletion — i.e. exactly today's behavior.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The diff-line parser needs to walk the unwrapped diff buffer upward from a
clicked/selected row, but the row index it's handed is a view line index (which
counts wrapped lines). Expose the existing internal mapping (viewLines[y].linesY)
so callers can translate, with the same lock and stale-tail guard that
HyperLinkInLine uses against a concurrent shorter re-render.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The patch package can already map a patch line index to its new-file line
number and back (LineNumberOfLine / PatchLineForLineNumber). The diff-line
parser being built on top of this needs the old-file equivalents: a deletion
has no distinct new-file position (two consecutive deletions share one), so to
land on the exact deletion when diving into staging we have to look it up by
old-file line number.
Add OldLineNumberOfLine and PatchLineForOldLineNumber as direct mirrors of the
new-file functions, counting DELETION+CONTEXT lines instead of ADDITION+CONTEXT.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Captures a design discussion about the remaining big problem behind the
focused-main-view feature: recovering a diff row's patch-space identity
(file, type, source line) when the rendering came from a pager. Records
the two complementary mechanisms (a host-side parser for
structure-preserving renderings, and a pager-emitted OSC protocol for
ones that restructure), the per-cell carrier with its keyboard/mouse
access rules, the type + old?/new-line payload and why each field is
load-bearing, and the version-negotiation handshake. Design only; no
implementation yet.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Diving into staging or patch building from a focused main view (by
double-clicking a line, or pressing enter on the selected line) always
landed on a single-line selection. Entering the same views through the
side panel honours the UseHunkModeInStagingView config and selects the
whole hunk by default. The two ways in should agree, so that diving in
from the main view feels like the established flow.
A non-negative line index in NewState was overloaded for two intents:
clicking directly on the patch explorer view (where a single-line range
is the start of a drag) and diving in from the main view (where we want
the default select mode). Distinguish them with SelectLineInDefaultMode
on OnFocusOpts: the main-view entry points set it; the click-to-drag
path does not.
In hunk mode the selection covers the block of changes around the
clicked line. A context line has no surrounding changes, so we snap to
the next change line (as toggling hunk mode does); the clicked context
line itself is then not part of the selection.
This is a separate commit only because the branch is a throwaway
prototype; in a real history it would be folded into the commit that
introduces the focused-main-view enter behavior rather than landing on
top of it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Records session 3: the cmd/pty scroll-restore mechanism, the refreshMainViews
reset reorder, the corrected onNewKey understanding, and the remaining
timing-race investigation to do before productionizing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
EscapeFromPatchExplorer re-renders the side panel's content back into the main
view and wants to land at the scroll position and selection the user had before
diving into staging/patch building. The previous version set the origin on the
next UI tick, after the placeholder had already been painted at the wrong
position, so the restore was visible as a jump.
Instead, ask the re-render itself to restore the scroll (via
ScrollToOriginYForNextTask), so the saved position is applied in the first
paint that shows the real content. The selection still needs the diff loaded
down to the selected line, so restore it via ReadToEnd once the content is
fully read; the scroll is no longer touched there.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When re-rendering content the user was already scrolled into, we want the
saved scroll position applied exactly when the real content first paints — not
before. Setting the origin up front instead paints it onto whatever placeholder
is currently in the view (e.g. the shorter buffer CopyContent left there),
which flickers: either a blank frame past the placeholder's end, or a jump to
the top when the task resets the origin at startup.
Add ViewBufferManager.ScrollToOriginYForNextTask: the next cmd/pty task then
(a) does not reset the view to the top at startup even though the command key
changed, so the placeholder stays put, (b) sizes its initial read to the saved
position so enough content is loaded to fill the view there, and (c) scrolls to
it as part of the first refresh, in the same paint that shows the real content.
This is the cmd/pty analogue of RenderStringWithScrollTask.
No caller sets it yet, so this is behaviour-preserving on its own.
Co-Authored-By: Claude Opus 4.8 (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 4.8 (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 4.8 (1M context) <noreply@anthropic.com>
Records session 2's findings: the escape flicker was caused by the layout's
scroll-up-to-fill clamp running against partially-loaded async content (not the
"only a screenful loaded" mechanism session 1 guessed); the full origin-reset
chain (onNewKey / CopyContent / layout clamp) and how each is handled; the three
standalone bug fixes that landed; and the precise remaining task (apply the
saved origin at the pty task's first repaint, i.e. a cmd-task RenderWithScroll).
Also captures the reusable debug tooling that was stripped from the tree.
Co-Authored-By: Claude Opus 4.8 (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 4.8 (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 4.8 (1M context) <noreply@anthropic.com>
HyperLinkInLine read v.lines/v.viewLines without holding writeMutex, so it
could race a concurrent re-render rebuilding the buffer. It also indexed
v.lines by viewLines[y].linesY after only checking y against len(viewLines);
since refreshViewLinesIfNeeded overwrites viewLines in place without
truncating, the tail can hold stale entries pointing past a shrunk v.lines,
giving an out-of-range panic while a shorter diff is still loading.
Take writeMutex (as the sibling view methods do) and bounds-check linesY
against len(v.lines), returning "no link" rather than panicking.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Instead of a user config that always shows a selection on focus, the
focused main view starts without a selection (matching master). Pressing
<space> shows a selection in the middle of the view (no scrolling);
pressing <esc> hides it again before falling through to exiting the view.
This was already possible, but only when a file was selected, and it woudln't
always land on the right line when a pager was used. Now it's also possible to
do this for directories, and it jumps to the right line.
At the moment this is a hack that relies on delta's hyperlinks, so it only works
on lines that have hyperlinks (added and context).
The implementation is very hacky for other reasons too (e.g. the addition of the
weirdly named ClickedViewRealLineIdx to OnFocusOpts).
