A few comments still reasoned in terms of SYNC vs ASYNC refreshes, a
distinction that no longer exists: sync vs async is now derived from the
calling thread. Restate them in terms of the current mechanisms
(RefreshFromWorker blocking its worker, model updates being enqueued on
the UI thread) without changing any behavior.
Co-Authored-By: Claude Fable 5 <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>
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>
Operations that check something out (checkout, create branch, move
commits to a new branch, fetch-and-checkout) selected the newly
checked-out branch by calling SelectFirstBranchAndFirstCommit() before
the refresh and passing KeepBranchSelectionIndex so the refresh wouldn't
override it. That set the selection directly, usually from a worker
goroutine (WithWaitingStatus/WithInlineStatus). Now that the refresh's
own selection write is bounced onto the UI thread, the two writes could
land in either order, and under load the refresh's "restore the
previously-selected branch" write would win — leaving the old branch
selected instead of the new one (flaky
move_commits_to_new_branch_from_base_branch).
Replace it with declarative selection intents applied inside the
refresh's own bounce, so the selection is set on the UI thread and
atomically with the list write (no off-thread write, and no BLOCK_UI
needed to avoid a flicker):
- BranchSelection: SelectCheckedOutBranch selects the checked-out branch
(top of the list). The default, KeepBranchSelectionByName, restores
the previously-selected branch by name as before. This replaces the
KeepBranchSelectionIndex bool.
- CommitSelection: SelectHeadCommit (already existed) for the commit.
- SelectTopReflogCommit selects the top reflog entry, since a checkout
adds a new entry there (reflog/checkout relies on this).
SelectFirstBranchAndFirstCommit is gone. The previously-selected branch
is now read at the top of the branches bounce, before the list is
overwritten, so that read moves onto the UI thread too.
fetchAndCheckout's refresh changes from ASYNC to SYNC so its
post-refresh focus switch can run in Then on the UI thread; SYNC keeps
the inline fetch spinner spinning (only BLOCK_UI would freeze it).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
refreshRemotes now loads the remotes on the worker and writes
Model.Remotes, rebuilds the pull-requests map, and updates the selected
remote's RemoteBranches inside an onUIThreadUnlessRepoChanged bounce.
RemotesController.addAndCheckoutRemote read Model.Remotes right after its
SYNC REMOTES refresh to select the newly-added remote; since that write
now bounces, the selection (and the follow-up fetch) move into Then so
they run against the post-refresh model rather than the stale one.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Operations that show an inline status ("Pushing", "Fast-forwarding",
"Fetching", …) removed it by relying on the async refresh they trigger
to redraw the view after the item operation had been cleared. That
ordering was never guaranteed: the item operation is cleared on the
worker once the operation's function returns, while the refresh redraws
the item from the UI thread whenever its (asynchronous) git work
happens to finish. If the refresh redrew before the clear, the status
was left on screen with no later redraw to remove it, so the branch (or
tag/remote) stayed stuck showing e.g. "Pushing" indefinitely even though
the operation had completed. This is timing-dependent, which is why it
surfaced as rare, hard-to-reproduce reports and as flaky CI failures.
Fix it by re-rendering in stop() right after clearing the operation,
and by making these refreshes synchronous rather than async. Because a
synchronous refresh has already updated the model and queued its own
redraw by the time stop() runs, and UI-thread callbacks run in order,
the redraw we queue here runs last and draws the up-to-date model with
the status removed. An async refresh couldn't give that guarantee: its
model update might not have landed yet, so the redraw could briefly
flash the pre-operation status.
Pull refreshes through the shared CheckMergeOrRebaseAndSelectHeadCommit,
so that helper becomes synchronous too; its only other caller,
RegularMerge, thereby also refreshes synchronously, which is fine: a
synchronous on-worker refresh is what we want anyway.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The remote-branches context is a transient guest that takes over a host
window when you drill into a remote. SubCommits and CommitFiles already
adopt their parent's window via SetWindowName when shown; RemoteBranches
relied instead on its static window name ("branches") matching the remotes
context's window. That assumption only holds while remotes lives in the
branches panel. Adopt the parent's window like the other transient guests
so remote branches render in the right place once panels are configurable.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
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.
When this was originally introduced, it handled single clicks on a list entry
(treating them similar to a double-click by checking whether the click was on
the selected entry). Arguably it should have been called OnDoubleClick back then
already; but when we later changed it to do actual double-click detection (see
37197b8e9a), we should have renamed the methods.
We want to do this whenever we switch branches; it wasn't done consistently
though. There are many different ways to switch branches, and only some of these
would reset the selection of all three panels (branches, commits, and reflog).
Refresh is one of those functions that shouldn't require error handling (similar
to triggering a redraw of the UI, see
https://github.com/jesseduffield/lazygit/issues/3887).
As far as I see, the only reason why Refresh can currently return an error is
that the Then function returns one. The actual refresh errors, e.g. from the git
calls that are made to fetch data, are already logged and swallowed. Most of the
Then functions do only UI stuff such as selecting a list item, and always return
nil; there's only one that can return an error (updating the rebase todo file in
LocalCommitsController.startInteractiveRebaseWithEdit); it's not a critical
error if this fails, it is only used for setting rebase todo items to "edit"
when you start an interactive rebase by pressing 'e' on a range selection of
commits. We simply log this error instead of returning it.
Apparently this was an attempt at working around go's lack of default arguments,
but it's very unidiomatic and a bit confusing. Make it a normal parameter
instead, so all clients have to pass it explicitly.
This might seem controversial; in many cases the client code gets longer,
because it needs an extra line for an explicit `return nil`. I still prefer
this, because it makes it clearer which calls can return errors.
This adds a bunch of tooltips to keybindings and updates some keybinding descriptions (i.e. labels).
It's in preparation for displaying more keybindings on-screen (in the bottom right of the screen),
and so due in part to laziness it shortens some descriptions so that we don't need to manage both
a short and long description (for on-screen vs in-menu). Nonetheless I've added a ShortDescription
field for when we do want to have both a short and long description.
You'll notice that some keybindings I deemed unworthy of the options view have longer descriptions,
because I could get away with it.
Something dumb that we're currently doing is expecting list items
to define an ID method which returns a string. We use that when copying
items to clipboard with ctrl+o and when getting a ref name for diffing.
This commit gets us a little deeper into that hole by explicitly requiring
list items to implement that method so that we can easily use the new
helper functions in list_controller_trait.go.
In future we need to just remove the whole ID thing entirely but I'm too
lazy to do that right now.
We want to show an error when the user tries to invoke an action that expects only
a single item to be selected.
We're using the GetDisabledReason field to enforce this (as well as DisabledReason
on menu items).
I've created a ListControllerTrait to store some shared convenience functions for this.
The only time we should call SetSelectedLineIdx is when we are happy for a
select range to be retained which means things like moving the selected line
index to top top/bottom or up/down a page as the user navigates.
But in every other case we should now call SetSelection because that will
set the selected index and cancel the range which is almost always what we
want.
The global counter approach is easy to understand but it's brittle and depends on implicit behaviour that is not very discoverable.
With a global counter, if any goroutine accidentally decrements the counter twice, we'll think lazygit is idle when it's actually busy.
Likewise if a goroutine accidentally increments the counter twice we'll think lazygit is busy when it's actually idle.
With the new approach we have a map of tasks where each task can either be busy or not. We create a new task and add it to the map
when we spawn a worker goroutine (among other things) and we remove it once the task is done.
The task can also be paused and continued for situations where we switch back and forth between running a program and asking for user
input.
In order for this to work with `git push` (and other commands that require credentials) we need to obtain the task from gocui when
we create the worker goroutine, and then pass it along to the commands package to pause/continue the task as required. This is
MUCH more discoverable than the old approach which just decremented and incremented the global counter from within the commands package,
but it's at the cost of expanding some function signatures (arguably a good thing).
Likewise, whenever you want to call WithWaitingStatus or WithLoaderPanel the callback will now have access to the task for pausing/
continuing. We only need to actually make use of this functionality in a couple of places so it's a high price to pay, but I don't
know if I want to introduce a WithWaitingStatusTask and WithLoaderPanelTask function (open to suggestions).
The remote branches controller was using its own escape method meaning it didn't go through the flow of cancelling
an active filter. It's now using the same approach as the sub-commits and commit-files contexts: defining a parent
context to return to upon hittin escape.
We have not been good at consistent casing so far. Now we use 'Sentence case' everywhere. EVERYWHERE.
Also Removing 'Lc' prefix from i18n field names: the 'Lc' stood for lowercase but now that everything
is in 'Sentence case' there's no need for the distinction.
I've got a couple lower case things I've kept: namely, things that show up in parentheses.
This begins a big refactor of moving more code out of the Gui struct into contexts, controllers, and helpers. We also move some code into structs in the
gui package purely for the sake of better encapsulation
Stefan HallerandClaude Fable 5
Karol ZwolakandStefan Haller
Jesse Duffield
Maria José SolanoandStefan Haller
Moritz HaaseandJesse Duffield