Originally I thought we'd benefit from this change in this branch; turns
out that we didn't after all, because we changed the approach, but it's
a nice cleanup anyway, so we include it here.
This fixes two problems:
1. the list selection updated before the list content did, which caused
a bit of a wobble effect in the list
2. the main view would first update to a different commit's diff and
then back to the one that is being moved, resulting in a very ugly
flicker especially when moving the todo multiple times with auto repeat
Ever since scrolling the selection into view became opt-in, we have been
fixing the same class of regression by hand, five times so far: a
controller moves the selection somewhere new, doesn't say that it wants
the view to follow, and the selection ends up off screen. The decision
needs facts from two places — whether the selection went somewhere new is
known to the list, whether the scroll position is the caller's to manage
is known to the caller — and asking every caller for both is what keeps
going wrong. The callers that get it wrong are usually not even the ones
that moved the selection: they are pass-throughs like postRefreshUpdate,
which can't know what a refresh did to the selection.
So default to scrolling, and let the two callers that maintain the scroll
position themselves say so.
The one case where scrolling is always wrong is a refresh that no user
action is behind: a background poll, or a reload of state on window
focus, after a subprocess, or after a repo switch. Those must leave the
viewport wherever the user last scrolled it to — that is what made the
scrolling opt-in in the first place. Both are already marked in
RefreshOptions, so the refresh can decide it once, centrally, instead of
each caller judging it.
A user action that ends in a foreground refresh does now yank the view
back to the selection if the user had scrolled away from it. That's a
behaviour change, and there may be actions where it turns out to be
unwelcome; those we can fix individually, and it beats the ones that
don't scroll today.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Blocking keyboard input and hiding the working tree state mode are two
separate concerns; they were fused into one helper because every caller
so far wanted both. A caller that blocks input for something other than a
rebase would then hide the "Rebasing" indicator for the duration of its
operation, which has nothing to do with it.
Make it an explicit option instead, so blocking input on its own doesn't
imply anything about the modes on display.
Moving commits runs a rebase, which can take a while. Instead of
letting the drop indicator vanish the moment the button is released,
keep it in place and turn it into a "moving commits here" spinner once
the move takes longer than a short grace period, so that quick moves
stay free of flicker. The indicator is cleared when the post-move
refresh lands.
While a commit drag is in progress, escape now aborts it: the drag
state and the drop indicator are discarded and the mouse capture is
released, so nothing happens when the button is eventually released.
Otherwise escape keeps its normal meaning.
Reuse the drag autoscroller for commit drags. Scrolling stops once the
insertion point reaches the end of the allowed range in the scroll
direction, so during a rebase the view doesn't keep scrolling once the
last insertion position among the todos has been reached.
Pressing the left button on the current selection now starts a drag
that moves the selected commits, both in the normal commits view and
for todos during an interactive rebase. A press anywhere else falls
through to the usual click handling, so dragging from an unselected
line still creates a range selection, and releasing without having
moved collapses the selection to the pressed commit like a plain click
would.
While dragging, the insertion point follows the pointer: rows below
the dragged block insert after the pointed-at commit, rows above it
insert before it, and during a rebase the destination is limited to
the contiguous block of movable todos around the selection. gocui
moves the view cursor along with the pointer, so each drag event moves
it back to keep the original selection highlighted.
The move happens on release. The model may have been refreshed during
the drag, so the dragged commits are located again by their identity
(hash, subject, todo action); if they no longer form a unique
contiguous block, the drop is ignored rather than guessing.
Let the todo-move primitives take a distance instead of hardcoding a
single row, by iterating the one-row move in memory. Dropping a commit
several rows away thus rewrites the todo file once and, outside of an
interactive rebase, runs a single rebase rather than one per row.
Merge the up/down variants of the move commands into one
direction-parameterized implementation. Dragging commits is about to
need moves over arbitrary distances, which we don't want to build twice.
Moving a todo rewrites the todo file and advances the selection
synchronously, but the commits model is only rebuilt by the refresh. A
second press arriving before that grabs the swapped-with todo from the
stale model at the advanced index and moves it back, so holding the key
to move a todo several slots misbehaved. Use RefreshBlockingInput so the
second press is buffered and replayed once the moved todo list is in
place.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
edit, quick-start rebase, drop, reword, squash, fixup, amend
(including the amend-attribute author operations) and
discard-file-from-commit all run a rebase on a worker. A key pressed
while one is in flight could act on a stale commit or todo — pressing e
to start an interactive rebase, then up+d before it finishes, is the
motivating example. Switch them from WithWaitingStatus to
WithWaitingStatusBlockingInput so input is held and replayed against the
post-operation state, matching the commit-surgery ops that were already
sync.
Left alone: the custom-patch move/delete/pull-into-commit rebases (no
need to block input while building and applying a patch), the
loading-more-commits and patch-building toggle spinners (no rebase to
disrupt), and fetches and other non-surgery operations where blocking
navigation would only get in the way.
Move, revert, squash-fixups, create-fixup and cherry-pick paste ran
their rebase synchronously on the UI thread via WithWaitingStatusSync,
which froze the UI for the duration but kept the user from disrupting the
operation with a stray keypress. Switch them to
WithWaitingStatusBlockingInput so the git work runs on a worker — the UI
keeps rendering and the spinner animates — while input stays blocked for
the whole operation, as before.
discard-patch-from-commit also moves off WithWaitingStatusSync, but as a
plain WithWaitingStatus: it's a custom-patch command, and those don't
block input.
The bodies now follow the worker conventions: model state they need is
captured on the UI thread before dispatching, self.c.Refresh becomes
RefreshFromWorker, and CheckMergeOrRebase uses the worker variant. An
operation that moves the selection does so in the refresh's Then, so it
lands in the same frame as the refreshed commit list; squash sets it as
an absolute index there, because the shorter list would clamp a relative
move.
It reads the selected index and the commits and branches models to
decide where to move the fixup commit. Take those as parameters,
captured on the UI thread by the callers, so the function can run its
rebase on a worker without reading the model there. No behavior change;
the callers still run on the UI thread for now.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
With sync vs async now derived from the calling thread, the Mode field
and its SYNC/ASYNC constants no longer carry any information: Refresh is
always async, RefreshFromWorker always sync. Drop the field, the type,
and the Mode argument at every call site, and reduce the debug log's
mode name to a plain sync/async derived from calledFromWorker.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BLOCK_UI ran the whole refresh on the UI thread and parked it in a
wg.Wait for the duration, so the UI (and its spinner) froze while the
git work ran. Blocking the UI was never the point — the point was to
apply all the scopes' updates in one frame instead of a per-scope
cascade — and if we genuinely wanted to block input it should span the
whole operation, not just its refresh, which needs a gocui-level
mechanism we don't have.
So drop the mode and add a BatchUIUpdates option that achieves the
"one frame" effect without blocking: each scope's UI-thread bounce is
collected into a shared refreshBounceBatch during the refresh, and once
every scope has finished they're all applied inside a single OnUIThread
task. gocui drains every queued event before it redraws, so one task
means one repaint. The refresh itself now runs SYNC — on a worker when
issued from one (checkout, move-to-new-branch, the rebase-edit result
handling), so the UI thread stays live and the spinner keeps animating.
The batch needs a mutex because the scopes add concurrently from their
worker goroutines, and a closed flag so that any bounces enqueued after
the flush starts — the nested ones a flushed bounce produces in turn,
e.g. scrolling the selection into view — are dispatched immediately as
ordinary follow-ups rather than collected into a batch that nothing
will drain.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
interactiveRebaseWithFlag and dropMergeCommit ran inside the
WithWaitingStatus worker but read Model().Commits and wrote the
selection (SetSelection(startIdx)) there, racing the UI thread. Thread
the commits slice in from each caller, and hoist the pre-rebase
selection into a UI-thread helper (selectRebaseResultCommit) called
before dispatching — squash/fixup unconditionally, drop only on the
non-merge path, matching the previous action guard.
handleReword, amendTo, and the reset/set/add-co-author handlers pass
Model().Commits (and the selected line index) to a git rebase from
inside the WithWaitingStatus worker, racing the UI thread's model
writes. Read them on the UI thread before dispatching.
The author handlers index the full commit list by absolute start/end, so
the range sub-slice withItemsRange hands amendAttribute is not what they
need; capture the full Model().Commits there and thread it through.
CheckMergeOrRebaseWithRefreshOptions refreshes after a merge/rebase step,
and until now always via the UI-thread Refresh. Most of its callers are on a
worker (the WithWaitingStatus/WithInlineStatus merge, squash-merge, rebase,
pull, amend, drop, and patch-move handlers), so that refresh reads the
commits scope off the UI thread — the race the previous commit addresses for
everything else.
Split it: the default is for worker callers and refreshes via
RefreshFromWorker; a new CheckMergeOrRebaseWithRefreshOptionsFromUIThread is
for the handlers that run the step synchronously on the UI thread
(WithWaitingStatusSync, kept sync so rapid key presses batch): move up/down,
revert, squash-fixups, cherry-pick paste, and patch-discard.
The two share a private impl carrying which thread the caller is on, and the
auto-skip recursion (genericMergeCommandImpl for an empty commit) threads it
through so the follow-up step refreshes on the same thread. The
merge-and-commit refresh in SquashMergeCommitted, also on a worker, moves to
RefreshFromWorker to match.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A commits refresh does its git work on a worker and then reads the model,
the contexts, and the modes for that work directly from there:
LocalCommits.GetSelectionRangeAndMode/GetLimitCommits/GetShowWholeGitGraph,
Model.Commits/MainBranches/HashPool, the filtering path/author. Those are
owned by the UI thread, which is concurrently running the cursor and render
code, so the reads race it — the dominant, confirmed source of the
commits-scope flakes (the startup ClampSelection vs GetSelectionRangeAndMode
race, for one).
Gather them into an immutable capturedCommitState on the UI thread, before
the git work is dispatched, and have refreshCommitsWithLimit compute from
that snapshot. UI-thread callers capture inline; worker callers can't (a
SYNC/BLOCK_UI refresh parks the UI thread at wg.Wait, so hopping from a
scope sub-worker would deadlock), so the capture is lifted out of the scope
worker into the refresh orchestration, and worker callers announce
themselves with a new RefreshFromWorker entry point that hops the capture to
the UI thread and blocks for it (OnUIThreadAndWait). BLOCK_UI runs the whole
refresh on the UI thread regardless of the caller, so it captures inline
too.
Every refresh issued from a worker that reaches the commits (or branches,
which pulls in commits) scope is converted: the fast-forward, branch/tag
delete, worktree remove/detach, push, reword-via-rebase, author edits,
custom-command, hard-reset-with-autostash, reset-to-ref, fetch-and-checkout,
gpg-stream, post-fetch, and external-change-poller refreshes, plus the
branch checkout and move-commits-to-new-branch refreshes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This is preparation for upcoming commits that will bounce refresh-scope
model updates (e.g. Model.Files) onto the UI thread by enqueuing the
write via OnUIThread instead of applying it directly on the worker
goroutine. Once that lands, a Then callback that reads the model must
run after that queued write has been processed, not synchronously at
wg.Wait() time — at that point the workers have returned, but a bounce
they queued may not have been processed yet.
Queuing Then via OnUIThread here, ahead of that change, guarantees the
right ordering once it lands: a bounce queued earlier in the same
refresh is already sitting in the channel by the time wg.Wait()
returns, so Then enqueued after it will always be processed after, and
see the post-refresh model.
The signature change to func() error lets Then propagate errors
through gocui's normal error handler (the same path key-handler errors
take).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Setting a single commit to "edit" ran the interactive rebase inline on the
UI thread with no spinner, while its sibling startInteractiveRebaseWithEdit
(used when editing multiple commits or quick-starting a rebase) already runs
on a worker with a waiting status. Make the direct path match.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Starting an interactive rebase (the `edit` command and quick-start) used
to capture the selected commit range by hash before starting the rebase
and restore it afterwards, because new update-ref lines for stacked
branches can shift the commits' positions in the list. The generic
keep-selection-by-hash mechanism now does exactly this for every refresh,
including these, so the bespoke code is redundant.
This relies on the previous commit, which taught the generic matcher to
handle the case where the selected commit turns into a rebase TODO entry
while it's being edited - something the bespoke code handled implicitly
by matching on hash alone.
With the recently added external change detection, it happens more often
now that we refresh the commits list because an agent made a commit in
the background. In this case, if we keep the selection index the same,
it now points at a different commit, making the main view show a
different commit too, which is confusing and annoying. To fix this,
track the selected commit and range anchor by hash before reloading,
then restore those rows if both hashes still exist. This also allows us
to get rid of some bespoke code that did this for the specific cases of
reverting a commit or cherry-picking commits, because those are now
handled by the generic mechanism.
Until now every keybinding config field was a plain string. That meant a user
couldn't ask for two keys to invoke a command — the config silently accepted
only one form.
Convert every string-typed field across all 13 KeybindingXxxConfig structs to
Keybinding so the union type extends to every command. Defaults wrap their
single-key value in Keybinding{...} so the generated Config.md still renders one
scalar key per binding.
The alt fields keep their separate Binding registrations for now: this commit
does not yet introduce the merge mechanism that folds them into the main field —
that comes in a follow-up. Consumers previously calling opts.GetKeys on a string
field now call opts.GetKeys on the Keybinding, or take .String() / Keys[0] where
a single value is needed.
Adds a Keybinding.String helper for rendering, schema-generator work that
inlines the Keybinding union into each consuming property, and a unit test
covering the user-facing scalar/sequence YAML forms for quit.
Constructing a menu item key from a literal character requires
gocui.NewKeyRune('r'), which is a bit noisy. Add a private menuKey helper in
both the controllers and helpers packages so the common case in either reads as
menuKey('r'). Duplicating the one-liner is cheaper than a cross-package import
dependency and avoids forcing every controller file to qualify the call.
The reason for doing this now is that we are going to change MenuItem.Key to a
slice of keys later in the branch, which means we'd have to add `[]gocui.Key{`
at each call site, making them even more noisy. With the menuKey helper we can
just change its signature and leave all clients unchanged.
This bundles the keyName and a rune, so that we don't have to pass these around
separately everywhere. This should make it easier to swap out the rune for a
string when we upgrade to tcell v3.
I copied all files except dot files (.github and .gitignore), the _examples
folder, and go.mod/go.sum.
At some point we may want to copy the files back to the gocui repo when other
clients (e.g. lazydocker) want to use the newer versions of them.
For the branches panel we might consider unifying it with the existing `o`
command for creating a PR: it could check if there is a PR already, and open it
if so, or create a new one if not.
However, I also want the command in the local commits panel for the checked out
branch, and there's no existing "Create PR" command there; and the `o` command
opens the selected commit in the browser, so it's unrelated.
I've optimised for muscle memory backwards compatibility here:
- Outside interactive rebase: press 'f' then instead of a confirmation
panel, a menu appears where you can choose to keep the selected commit's
message
- Inside interactive rebase: press 'f' then press 'c' to see the menu
for keeping the message, where if you press 'c' again it will retain the
current message. so 'fcc' is the chord to press.
We're also now showing the -C flag (which is what enables the behaviour)
against the todo.
I've picked the 'c' keybinding because 'C' was taken and it corresponds
to the flag. Previously that showed a warning about a change in
keybinding for cherry picking but it's been ages since we've made that
change so I'm happy to retire it.
Not used yet, we pass an empty string everywhere, to match the previous
behavior. Just extracting this into a separate commit to make the next one
smaller.
Co-authored-by: Stefan Haller <stefan@haller-berlin.de>
It is possible to scroll the selection out of view using the mouse wheel; after
doing this, it would sometimes scroll into view by itself again, for example
when a background fetch occurred. In the files panel this would even happen
every 10s with every regular files refresh.
Fix this by adding a scrollIntoView parameter to HandleFocus, which is false by
default, and is only set to true from controllers that change the selection.
We move the selection down by the number of commits that were reverted (to keep
the same commits selected). However, this only happens after refreshing, which
has rendered the main view with the wrong commit, so we need to render it again
after moving the selection.
There are many other callers of MoveSelection in LocalCommitsController, but all
of them happen before the refresh. Revert is special because it needs to move
the selection after refreshing, e.g. when reverting the only commit of a branch.
Stashing doesn't affect submodules, so if you have a working copy that has
out-of-date submodules but no other changes, and then you revert or paste a
commit (or invoke one of the many other lazygit commands that auto-stash, e.g.
undo), lazygit would previously try to stash changes (which did nothing, but
also didn't return an error), perform the operation, and then pop the stash
again. If no stashes existed before, then this would only cause a confusing
error popup ("error: refs/stash@{0} is not a valid reference"), but if there
were stashes, this would try to pop the newest one of these, which is very
undesirable and confusing.
There's no reason not to allow these.
Technically we could enable a few more, but I chose not to because some might be
surprising or confusing in filtering mode. For example, creating a fixup commit
would work (shift-F), but the newly created commit might not show up if it
doesn't match the filter. Similarly, pressing `f` to fixup a commit into its
parent would work, but that parent commit might not be visible, so users might
expect to be fixing up into the next visible commit.
CheckMergeOrRebase calls Refresh already. However, it does an async refresh by
default, so we must turn this into a sync refresh so that moving the selection
down by one works even for the very first commit in history. Also, we must add
an explicit call to FocusLine so that the view selection is in sync with the
model selection; previously this was taken care of by the PostRefreshUpdate call
that happens as part of a refresh.
