Remove the BLOCK_UI refresh mode#5790
Open
stefanhaller wants to merge 20 commits into
Open
Conversation
Agents (and humans new to the repo) repeatedly go looking for the gocui sources in go.mod, go.sum, or the module cache and hit a dead end, because gocui is a fork maintained in-tree under pkg/gocui rather than pulled in as a dependency. Record that in AGENTS.md so the dead end is avoided. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
It always returned nil.
…mbda All clients pass a function that returns nil.
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>
This was useful when there was a BLOCK_UI mode where f() was called differently, but now we no longer need it. I'm making this change as a separate commit because folding it into the previous one (which would conceptually have made sense) would have made that diff unreadable because of the indentation change. The variable `fRunsOnUIThread` and its comment no longer make sense now; we'll clean this up next. The diff is best viewed with --ignore-all-space.
There is no f() function any more, so a variable named "f runs on" doesn't make sense. And we also don't need it any more; it used to be necessary when its meaning was not exactly the same as `!calledFromWorker`, but also included the BLOCK_UI case, but that has changed several commits ago.
Then, and BatchUIUpdates, previously only worked for a SYNC refresh: the calling goroutine blocked in wg.Wait until every scope had finished, and only then flushed the batch and ran Then. An ASYNC refresh had no such join point — it dispatched each scope onto its own worker and returned right away — so Then was forbidden (it would have run before the scopes finished) and a batch would never be drained. Give the async path a join of its own. Both paths now register their scopes in the WaitGroup, and the finishing work — wg.Wait, the batch flush, and Then — moves into a closure. A SYNC refresh runs it inline as before; an ASYNC refresh dispatches it to a worker, so the caller still returns immediately but the batch and Then run once every scope is done. Besides lifting the restriction, this makes SYNC and ASYNC differ only in whether the finishing work blocks the caller, which is what lets a later commit drop the mode entirely and key the choice off the calling thread instead.
Creating a branch checks it out, and checking out a distant ref (a tag or a commit far from HEAD) can take a noticeable while. NewBranch ran that synchronously in the prompt's confirm handler, on the UI thread, so the UI froze — no spinner, no repaint — until it finished. Move the branch creation (and the autostash path) onto a worker with a waiting status, mirroring CheckoutRef, and refresh from the worker so the UI thread stays live and the spinner keeps animating. Push the branches context from the refresh's Then rather than up front: the refresh already batches its UI updates, so switching panels there lands the switch in the same frame as the refreshed branch list instead of flashing the pre-refresh list while the checkout is still running. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Whether a refresh should block or run in the background was controlled
by the Mode field, but that always lined up with the calling thread: a
UI-thread Refresh must not block the UI, while a RefreshFromWorker runs
on a worker where blocking is exactly what we want. Now that Then and
BatchUIUpdates work regardless of that choice, drop Mode from the
decision and key it off calledFromWorker instead:
- Refresh (UI thread) runs its scopes and the finishing step (wait,
batch flush, Then) on workers, so the caller returns immediately —
what ASYNC used to mean.
- RefreshFromWorker runs them on the calling worker, blocking it until
everything is done — what SYNC used to mean.
Demos keep taking the blocking, inline path so everything still lands in
one deterministic frame.
In practice this flips the handful of RefreshFromWorker calls that
passed ASYNC — they now block their worker until the refresh finishes,
keeping the waiting-status spinner up until the UI actually updates —
and the many UI-thread refreshes that defaulted to SYNC, which no longer
freeze the UI thread while the git work runs. Mode now only feeds the
log line; the next commit removes it.
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>
Long-running operations that lazygit drives itself (rebases, and the commit surgery built on them) can be corrupted by keys the user presses while they run: pressing e to start an interactive rebase, then up+d before it finishes, must act on the resulting todo list, not race the rebase. WithWaitingStatusSync gets this today only as a side effect of freezing the UI thread, which the rest of this branch is moving away from. Add a nestable counter, BeginBlockingEvents/EndBlockingEvents, that withholds input at the event-dispatch layer without freezing anything: while blocked, key events are buffered and replayed in order once the count returns to zero (so they act on the now-current context), mouse clicks and hover are dropped (replaying them against a changed layout would target the wrong thing), and scrolling, resize, focus and all rendering keep flowing. These are the reusable core; a gui-level helper that brackets them around a worker operation follows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bracket gocui's BeginBlockingEvents/EndBlockingEvents around a worker operation that shows a waiting status. The block is begun synchronously on the UI thread, before the operation is dispatched to a worker, so no keypress can slip through in between; it ends via OnUIThread once the operation and its refresh have applied their UI updates, so the replayed keys act on the refreshed state. This composes what the retiring WithWaitingStatusSync did — show a status and block input — but on a worker, so the UI keeps rendering (spinner animates, model updates land) instead of freezing. Callers follow in subsequent commits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
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.
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.
With the last synchronous commit-surgery callers moved to workers, nothing runs CheckMergeOrRebase on the UI thread anymore, so CheckMergeOrRebaseWithRefreshOptionsFromUIThread has no callers. Remove it and fold the shared checkMergeOrRebaseImpl back into CheckMergeOrRebaseWithRefreshOptions, which is now always on a worker. The runAction closure loses its calledFromWorker parameter for the same reason. genericMergeCommandImpl keeps its calledFromWorker flag: the merge/rebase-continue subprocess path still runs on the UI thread when invoked straight from the menu, and on a worker for the recursive auto-skip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Nothing calls it anymore now that the commit-surgery operations run on a worker with input blocked. Remove the helper, its bespoke synchronous spinner loop (renderAppStatusSync/setAppStatusContent), the popup-handler plumbing, and the interface method. That loop was also the only thing suppressing the yellow "Rebasing" mode indicator (and its reset button) while lazygit drives a rebase itself. Move that suppression to WithWaitingStatusBlockingInput so it applies to every input-blocking commit-surgery op — including the ones that already ran on a worker (edit, drop, and so on) and previously let the indicator flash on mid-operation. It's cleared after the refresh, so an operation that legitimately leaves a rebase in progress still shows the mode. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The two branches of the `refresh` closure ran the scope function identically; they differed only in that the UI-thread path registered each scope as its own gocui task while the worker/demo path used a bare goroutine (and only the latter logged per-scope timing). Those per-scope tasks were redundant. performRefresh always runs under a task that stays busy until the wg.Wait in waitAndFinalize joins every scope goroutine: the calling worker's own task when called from a worker, or the waitAndFinalize worker task when called from the UI thread — and that task is created (busy) before the triggering event's task goes Done, so there is no window in which nothing is busy. Repo-switch safety and the integration-test idle signal are therefore already covered without giving each scope its own task. Collapsing to the single goroutine path also means the timing log now fires for UI-thread refreshes too, not just worker ones. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
For async refreshes (from UI thread) it would only log the time it took to schedule the refreshXxx calls, which is not useful.
Fetching pull requests can take a long time, and we don't want to delay the refresh by it; in particular, for a WithWaitingStatusBlockingInput we want the UI thread to be unblocked again while pull requests are still fetching in the background. This is similar to how we fetch the behind values for branches in BranchLoader; this will update the UI without much flicker when done, and doesn't have to block anything.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We used to run some refreshes with a BLOCK_UI mode that causes it to run on the main thread, blocking it. The two main reasons for doing this were that in some cases we want the UI to update all at once, rather then each panel when it's done refreshing (and in particular also render a selection change in sync with the list update, e.g. when checking out a branch, where the checked-out branch moves to the top and gets selected there), and that we wanted to block keyboard input while the refresh was running (so that people who like to work fast can "type ahead"; e.g. press
eto start an interactive rebase, and thenupanddto set the next rebase todo to dropped).There were two problems with this: it blocked UI updates, so for checking out a branch the status spinner would spin while the git checkout command is running, but then freeze while the refresh was running, which looked ugly; and also, for the "type ahead" scenario it isn't enough to block events only while refresh runs, but we'd have to block them during the entire operation.
This PR makes several improvements to this mechanism: