Add file and change-block ("hunk") navigation to the focused main view,
mirroring the staging view's hunk keys: `<left>`/`<right>` jump to the
previous/next hunk and `n`/`N` to the next/previous file. A "hunk" here is
lazygit's notion — a run of consecutive added/deleted lines separated by
context, not a git `@@` section — matching what the staging view jumps
between.
This is a consumer of the diff-line primitive in its forward direction:
resolve each rendered row's patch identity, then scan for the next/previous
change block (by the line type) or file boundary (by the path changing). The
file scan lands on the top of the neighbouring file even when a restructuring
pager leaves the header rows untagged, by backing up over them from the
file's first identifiable row — which is impossible without the per-line
metadata once the pager stops emitting a parseable unified diff. The
boundary arithmetic is pulled out into pure functions and unit-tested.
The anchor is the selected line if a selection is showing, else the top
visible line. With a selection we move it to the target and scroll it into
view, like the staging view; with none we stay in scroll mode, bringing the
target to the top without creating a selection.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Increasing or decreasing the diff context size (the `{`/`}` keybindings)
re-renders the diff with a different `git diff -U<n>` command. Because the
command key changes, the render reset the main view to the top — losing the
spot the user was reading, which is exactly the spot the context change is
about.
Preserve it instead, reusing the identity-based restore built for the escape
path. This is its sibling consumer: capture the lines around the anchor (the
selection, or the top visible line when there's none) as restore candidates,
and after the re-render land on the nearest one that survived, put back at the
same screen row. Prefer the anchor line itself, falling back outward only when
it didn't survive: a context line vanishes when the context size shrinks,
whereas additions and deletions always survive, so expansion stops at the
first change line in each direction and the candidate list always contains a
survivor. Landing on the nearest survivor keeps scrolling to a minimum, and a
context line that is still in the patch stays put (or stays selected).
This generalizes the shared restore helper to take a prioritized candidate
list (the escape path passes a single candidate). It covers the focused main
view and every side panel's diff, since they all render into the same "main"
view through the same path. A showing selection is re-established on the landed
line; otherwise the view stays in scroll mode.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The escape restore's mechanism — set a RenderRestore that scans the
re-rendering content for a target patch identity and, once it loads,
positions the view on the matched row — is about to gain a second caller:
preserving a diff view's scroll/selection when its -U context size changes
re-renders it (the sibling consumer of the diff-line primitive). That caller
positions the row differently (put it back where it was, rather than scroll to
and select it), so split the positioning out behind a `place` callback and
keep the scan/swap machinery shared.
Behaviour-preserving: RestoreFocusedMainViewOnEscape passes the same
FocusPoint-and-select closure the inline Apply used.
Co-Authored-By: Claude Opus 4.8 (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 task's first paint (the swap), alongside the restore that
already runs there: the previous content stays at its scroll until the new content
takes its place, then the new content appears at the top. A same-content re-render
keeps its scroll (no reset); a restore places the scroll itself. 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 (it no longer needs the task key or the pending
restore for this) into the read loop, driven by LinesToRead.ResetOrigin, which the
cmd/pty wrappers set from the key comparison the reset used to do. The manager's
onNewKey callback is renamed resetOrigin to match its now-decoupled timing.
onMouseMove (and findHyperlinkAt, which it calls) 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 onMouseMove's bounds check and findHyperlinkAt's 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. Pre-existing, but the off-screen
re-render rebuilds viewLines on the task goroutine more often, widening the window.
§14: the escape restore now anchors on a patch identity scanned from the loading
re-render (items 1+3 of the part-3 plan, which collapsed into one mechanism), the
partial §12.2 routing fix, and the analysis for the deferred pieces (the (b)
no-clobber lever's interaction with the entry origin reset, NormalSecondary
routing, the hyperlink-backend match limitation, the O(n^2) scan). Records that
interactive sign-off — including the still-pending session-5 scrollbar/stopped-task
checks — remains, since the agent couldn't drive the TUI.
Diving into staging from a focused main view records a snapshot so escape can
return there. But it was recorded only on the half we entered (unstaged), and
staging the last unstaged hunk moves the selection to the staged half
(RefreshStagingPanel pushes StagingSecondary when the unstaged state goes empty).
Escaping from there found no snapshot and fell back to the files panel, instead
of returning to the focused main view.
Record the snapshot on both staging halves at entry, and clear both on escape.
Escaping from either half now returns to the focused main view, and the
identity-based restore lands on the line the explorer ended up selecting — which,
with the last unstaged hunk gone, is shown in the main view (the file now has only
staged changes, so no split). Routing the split-and-tabbed-to-staged case to the
secondary focused main view is left for follow-up; see focused-main-view-notes.md
§14.3.
Escaping a patch explorer (staging / patch building) back to the focused main
view it was entered from used to replay a numeric scroll position and selection
index captured on the way in. But the reason to escape after staging or dropping
a hunk is that the content changed, so a saved index points at the wrong line —
and the host auto-advances the explorer's selection to a still-valid line anyway,
which is the line the user actually cares about returning to.
Restore by *patch identity* instead. On escape, read the (file, type, source
line) the explorer currently has selected, then have the main view's re-render
land on the row that matches it: scan the incoming content as it loads (the
inverse of the diff-line primitive), and once the matching row plus a screenful
below it have loaded, swap the off-screen render in and scroll to / select that
row in one step. FocusPoint with scrollIntoView centres the row only if it's
off-screen, so the common unchanged-content escape — where the row is already
where it was — doesn't move at all. If the line is gone (the content really
changed), nothing is forced.
This generalizes the scroll restore from a fixed origin to a predicate
(RenderRestore: FirstPaintReady decides when the saved position is reachable,
Apply re-establishes it), folding the separate selection restore into the same
first paint — so it no longer rides a post-load callback that could fire early.
The restore also now survives task replacement, which the numeric version did
not: a periodic refresh can stop the escape's re-render before it first-paints.
The pending restore is held on the buffer manager and is *not* cleared when a
task starts, so the replacement task picks it up. It is not gated on the command
key — staging the last unstaged hunk re-renders `git diff` as `git diff --cached`,
a different command, yet the line to land on is still in the new content — but
validates itself: the scan finds the target line only when the content still
contains it, so applying it to a different item is a harmless no-op. A task
clears it once it has applied it (found or not), so it lives for exactly one
re-render. Because the restore is anchored on content identity and is idempotent,
"survive replacement" and "restore by identity" are one mechanism, not two.
With the identity in hand the snapshot no longer needs the captured scroll/index;
they're derived from the explorer's live selection.
The escape restore (next commit) finds the row in a re-rendering focused main
view that matches a target patch identity, and it has to do so while the content
is still loading — i.e. against the off-screen buffer, before it is swapped in,
since the displayed buffer still shows the previous render.
Add the two primitives that scan needs:
- OffscreenDiffLineContents exposes the per-line diff material (text, metadata,
hyperlink) of the rows read so far into the off-screen render, so the resolver
built last commit can run against the incoming content.
- ViewLineForBufferLine maps a matched (unwrapped) buffer line back to the first
view line that renders it — the inverse of BufferLineForViewLine — so the
restore can scroll to and select that line once the render is swapped in.
The diff-line primitive (recover a rendered row's patch-space identity) is about
to gain a second, inverse consumer: the escape restore scans a focused main
view's rows as it re-renders, looking for the row that matches a target patch
identity. That scan runs over the *loading* off-screen buffer, not the displayed
view, so the resolver can't be tied to the displayed view's per-view-line readers.
Pull the three backends (OSC metadata, buffer parse, lazygit-edit hyperlink) onto
a single buffer-agnostic resolver that takes a snapshot of a diff's per-line
content — text, metadata and hyperlink per unwrapped buffer line — and the line
to resolve. The forward consumers (click/enter/edit/PR) feed it a snapshot of the
displayed buffer (gocui.DiffLineContents) after mapping the wrapped view line to
its buffer line; the upcoming scan will feed it the off-screen buffer's loaded
rows. Behavior is unchanged.
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, applying the saved scroll, 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.
Co-Authored-By: Claude Opus 4.8 (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, 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>
