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>
Checking out a remote branch that has no local counterpart creates the
local branch, refreshes, and then checks it out. The refresh exists so
that CheckoutRef finds the new branch in the model and attaches an inline
status to the branch item instead of showing a global waiting status. But
since UI-thread refreshes stopped blocking, the checkout started before
the refreshed branches had landed in the model, so the lookup failed and
we always got the waiting status. Run the checkout from the refresh's
Then, which is queued behind the model update.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
gui.git, gui.helpers and gui.State are all replaced on a repo switch,
which runs on the UI thread. The background fetch and the external-
change poller read them from their own goroutines, racing the
reassignment. This race can't show up in the integration suite, which
doesn't enable the background routines, so no -race run will ever flag
it; it can only bite real users who switch repos while a background
fetch or poll is in flight.
Capture the objects a routine iteration needs in a single blocking
UI-thread hop before using them, the same pattern the refresh's input
capture uses. For the fetch this has two welcome side effects: the
fetch, the post-fetch refresh's generation baseline, and the recorded
fetch time now all refer to the same repo (the old comment documented
the timestamp's mismatch as a known, unguarded race), and the git
instance the fetch runs through is pinned to that repo's directory, so
a switch mid-fetch can no longer direct in-flight work at the new repo.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PostFetchRefresh's refresh is the only background refresh carrying a
Then callback, and Then callbacks are not generation-guarded: when the
background fetch's refresh crossed a repo switch, the callback still
ran — in the new repo — and auto-forwarded the new repo's branches
because the old repo's fetch had completed. That was harmless in
practice (the update-ref call compares against the expected old value,
and it only does what the next fetch's auto-forward would do anyway),
but mutating refs in a repo whose fetch never happened is not an action
the user took. Skip the auto-forward when the repo generation changed
since the fetch started.
The generation is captured by the fetch's callers before the fetch
runs, not by PostFetchRefresh itself: the background fetch doesn't
block repo switching and is a network call, so by the time
PostFetchRefresh runs a switch may already have happened — a capture
there (or the one the refresh itself takes) would compare against the
new repo's generation and let the auto-forward through. For the manual
fetch the capture point makes no difference, since a foreground
operation blocks repo switching for its entire duration.
This deliberately guards only this call site rather than making Then
callbacks generation-guarded in general: a Then is an arbitrary
callback, and whether it is safe to skip on a repo switch is a decision
for the author of the call site.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The refresh workers read a few files at paths relative to the process
working directory: the submodule config read of .gitmodules, the files
refresh's check for conflict markers, and the submodule stash's
existence check. Git commands are pinned to the repo their instance was
created for, but these Go file reads still followed the cwd, so a
background refresh crossing a repo switch would read the new repo's
files while computing data for the old one. Join them with the worktree
root of the instance they belong to. (Most git-state file reads —
working tree state, rebase todos, bisect info — already resolve
against RepoPaths and need no change.)
This also fixes the submodule stash's existence check for nested
submodules: it stat'ed submodule.Path, which is relative to the parent
module, against the repo root — now it uses the submodule's full path,
matching the stash command right below it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The refreshes on focus-in, right after a repo switch, and after
returning from a subprocess are full foreground refreshes, so their
tasks kept Busy() true for as long as the slowest scope took — and any
switch attempt in that window was refused with the "can't switch"
toast. The focus-in one is particularly annoying: focusing lazygit is
often precisely what the user does in order to switch repos, and right
after regaining focus is when a refresh takes longest.
Blocking the switch bought nothing there. The refusal exists for user
operations, whose follow-up work (e.g. a Then callback reading the
model) isn't covered by the switch-safety guards; but these refreshes
merely reload state, and a refresh by itself is now switch-safe: its
git commands run against the repo it was started for, and the
generation guard drops its updates when the repo changed.
We can't just mark them Background, because that flag also decides
whether the files refresh lets git take optional locks to persist its
refreshed stat cache — worth doing for an attended refresh, and the
focus-in refresh (typically running right after external changes) is
the case that profits most. So split the two meanings: a new
DontBlockRepoSwitch option dispatches the refresh's tasks as background
tasks (excluded from Busy()) while keeping the attended optional-locks
behavior. Combining it with Then panics, since Then is not
generation-guarded.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An error returned from a gocui worker is shown to the user in an error
popup. For the branch loader's behind-counts worker that used to be the
"no such ref" popup when a background refresh crossed a repo switch:
the old repo's main branch didn't exist in the new repo. The previous
commits fix that scenario properly — the command now runs against the
repo the refresh was started for — but a stale worker can still fail
legitimately, most plausibly because that repo was deleted after
switching away from it (e.g. removing a worktree). Its results are
dropped anyway, so log the error instead of alarming the user about a
repo they already left.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A background refresh's model writes are dropped by the generation guard
when the repo is switched mid-flight, but its git commands kept running
— and because the refresh read the live git instance at each step, any
command issued after the switch ran against the new repo. Now that git
commands are pinned to the directory of the instance they were built
from, capture the instance once when the refresh starts and run every
scope's git work through it, so a switch-crossing refresh keeps
addressing the repo it was started for.
The instance is captured together with the repo generation, on the UI
thread (where repo switches run), so the pair can't straddle a switch:
an old instance paired with the new generation would compute data from
the old repo and write it into the new repo's model unguarded.
This also removes the refresh workers' unsynchronized reads of the live
instance pointer, which raced its reassignment on the UI thread when a
background refresh crossed a repo switch (foreground refreshes can't
cross one: they keep Busy() true, which refuses the switch).
Two reads keyed app-state by the live instance's repo path on a worker
and now use the captured instance, fixing which repo they file under
when crossing a switch: the pull-request cache, and the "user dismissed
the base-remote prompt" flag. The base-remote menu's handlers keep
reading the live instance: a switch dismisses any open popup, so they
can't run against the wrong repo (and the OnPress body runs under a
foreground task, which blocks switching anyway).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When the user picks a base remote in the "select remote repository"
prompt, we called setGithubPullRequests directly, bypassing the refresh
machinery — which meant hand-rolling the refresh env that call needs
(with a comment explaining why), and fetching against the branches
captured when the prompt was created. Issue a PULL_REQUESTS-scoped
refresh instead: it re-reads branches and remotes (both fast even in
large repos), fetches against those fresh values, and gets the refresh
machinery's guarantees without any special-casing. The config write is
re-read by the refresh from git config, so it is guaranteed to be
picked up.
The waiting status now covers the config write and the branches/remotes
reload, while the GitHub request itself continues as a background task
— which is how every other pull-request fetch behaves.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The watchdog only log.Fatal'd with a message, so a hung test told us
that it timed out but not where it was stuck -- useless for diagnosing
an intermittent deadlock under the race detector. Dump all goroutine
stacks to stderr first (the harness surfaces this process's stderr on
failure), turning a bare timeout into an actionable stack trace.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The integration test watchdog fails a test if its recording takes longer
than 40 seconds. Under the race detector everything runs several times
slower, so legitimately slow tests (e.g. a conflicting interactive
rebase) blow that budget and fail even though nothing is actually stuck.
Key the timeout off a build-tag constant: the `race` tag is set
automatically when the binary is built with -race, so a race build gets
a 5x-longer budget while a normal build is unchanged, and the two can't
drift apart the way a runtime flag would. The base 40s stays in one
place; only the multiplier varies by build.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Each status used to start a spinner render loop of its own, running on
a worker that inherited the foreground/background flavor of the
status's owner, and exiting only once the entire status stack was
empty. That shape had a real bug: a foreground operation's loop could
be kept alive by someone else's status. Finish a quick operation with
a waiting status while a background fetch's "Fetching..." status is
still showing, and the operation's render loop — a foreground worker
task — keeps ticking until the fetch ends. Busy() stays true for that
whole time, so repo switching is refused even though nothing is in
flight anymore; with a fetch hanging on a slow network, that means
minutes. The shape was also wasteful: overlapping statuses were each
drawn by their own loop (plus a duplicate whenever a task was paused
and resumed while another status was showing), all redundantly
redrawing the same top status.
Replace the per-status loops with a single loop owned by the status
stack as a whole: whoever shows the first status starts it, and it
exits after drawing a final empty frame once the last status is
removed. The claim/release methods on StatusManager keep the loop
flag's transitions atomic with the stack under the one mutex, so a
status added while the loop is about to exit starts a fresh loop
instead of going unrendered.
The loop always runs as a background task now: rendering issues no
git commands, so it never needs to block repo switching, and a
foreground operation's busy-ness is already carried by its own worker
task. This is what fixes the bug above, and it retires the need to
thread a foreground/background flag through the waiting-status
helpers altogether.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every full refresh includes the PULL_REQUESTS scope, and the worker it
spawns inherited the refresh's foreground/background flag. Full
foreground refreshes happen at startup, after switching repos or
worktrees, and when the terminal regains focus, so the GitHub API
request ran as a foreground task there, keeping Busy() true until it
completed. On a healthy network that's a few hundred milliseconds and
nobody notices; on a very slow one the request can stall for minutes,
and every attempt to switch repos in that window was refused with
"Can't switch repositories while an operation is in progress" even
though lazygit looked completely idle. (The request has no visible
status; at most, a background fetch hanging on the same bad network was
showing its "Fetching..." spinner, pointing the blame at the wrong
operation.)
The switch-safety guard only needs to wait for operations whose
remaining git commands would run against the wrong repo after a switch.
The pull-request fetch runs no git commands at all, and its model
writes are dropped when the repo generation has changed in the
meantime, so there is no reason for it to block switching.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The bounce model requires that a worker never touch UI-thread-owned
state: it should capture what it needs on the UI thread and pass that
in. Guard the two central accessors -- Model() (the git model) and
Context() (the context manager, which owns the mutable
current-context/stack) -- with a debug-only panic when they're called
off the UI thread. Since the integration tests run with -debug, a stray
worker access now fails deterministically and points at itself, rather
than surfacing later as a probabilistic data race.
One supporting change make the assertion usable: the integration test
driver inspects gui state from the test goroutine, so
GuiDriver.CurrentContext reads the context manager directly rather than
through the now-guarded c.Context().
Contexts() (the registry of context objects) is deliberately left
unguarded: workers legitimately fetch a context to grab its mutex or
check identity, so a blanket assertion there would flag safe accesses.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hasExecTodos reads Model().Commits. genericMergeCommandImpl evaluates it
when deciding whether to use a subprocess, and on the recursive auto-skip
path that runs on a worker -- so the read raced the UI thread. Bounce it
onto the UI thread there, keyed off the calledFromWorker flag the function
already carries (on the UI-thread entry path the read stays inline).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The file-path suggestions trie is rebuilt asynchronously and then read by
the suggestions search, which runs on an AsyncHandler worker. It lived in
Model().FilesTrie, so that worker read the (UI-thread-only) model. Move
it to an atomic pointer on the SuggestionsHelper instead: it's the only
place that uses it, the helper is recreated per repo (so the cache still
resets on a repo switch), and an atomic pointer is safe to store from the
build and load from the search worker.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "merge conflicts" refresh scope ran on a worker like the others, but
unlike them it does UI work rather than git work: RefreshMergeState reads
the current context and renders (or escapes) the merge-conflicts view.
Reading the context manager and rendering from a worker races the UI
thread. Bounce it onto the UI thread with onUIThreadUnlessRepoChanged,
exactly as the staging and patch-building scopes already do.
Running on the UI thread also lets EscapeMerge push the files context
directly instead of deferring the push to a separate UI task; it only
needs to drop the merge-conflicts mutex first, because the push
renders the newly focused file, which can take the mutex again. The
deferred push could lose a race against the same refresh's prompt to
continue the rebase/merge: if the prompt opened between
RefreshMergeState and the deferred push, the push declined to cover
the popup and was dropped, so closing the prompt landed the user in
the emptied merge conflicts view.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
RefreshSuggestions dispatched to an AsyncHandler worker that read
State.FindSuggestions and the prompt's TextArea (via GetPromptInput)
from the worker goroutine. The main thread rewrites both in
preparePromptPanel when it (re)creates a prompt panel, so an in-flight
suggestions worker races those writes -- two data races surfaced under
-race (filter_by_path/reword_commit_in_filtering_mode).
Capture both on the UI thread (RefreshSuggestions is only ever called
from UI-thread handlers) before dispatching to the worker. This is also
more correct: we search for the input as it was when dispatched, which
is what this request's AsyncHandler id corresponds to.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Dropping a range of stashes ran a refresh after each drop. A refresh
issued from the UI thread does its git work on a worker and applies the
model update in the background, so firing one per iteration let the
workers race: an earlier drop's refresh (which read a stash list that
still contained a later-dropped entry) could apply its result last,
leaving the stash view showing an entry that git had already removed.
Refresh once, after all the drops, so a single worker reads the final
stash list. The indices are captured up front and dropped highest-first,
so the remaining lower indices stay valid without an intervening
refresh. It's also cheaper: one `git stash list` instead of one per
entry.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The patch-building scope ran RefreshPatchBuildingPanel directly on the
refresh worker, where it read the commit-files selection and set the patch
view's origin off the UI thread — the latter raced the UI thread's draw.
Bounce it onto the UI thread, exactly as the staging panel just above
already does, guarded on the generation so a repo switch drops it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The test harness enqueued ErrQuit after a test finished, waited for the
program to go idle, then slept a fixed second and declared "gocui should
have already exited" if it hadn't. That fixed grace is fragile: under the
race detector the shutdown legitimately takes longer than a second, so
nearly every test failed with that message even though nothing was wrong.
Wait for the main loop to actually return instead. gocui now closes a
loopExited channel when MainLoop exits, and the harness blocks on it; the
existing 40s watchdog still fails a test whose loop genuinely never quits.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PopupMutex guarded CurrentPopupOpts against a popup being created on a
worker goroutine while the UI thread deactivated it, or reset it on a
repo switch. Now that popup and menu creation is bounced onto the UI
thread, every access to CurrentPopupOpts — create, deactivate, and the
reset-on-switch (which already runs on the UI thread) — happens on the
one goroutine, so the mutex protects nothing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Raising a popup or menu pushes a context and mutates the popup views, so
it must happen on the UI thread. But it can be triggered from a worker
goroutine — for example a WithWaitingStatus handler that hits a merge
conflict and calls PromptForConflictHandling, or a worker that shows a
confirmation — where it raced the UI thread's layout and draw code.
Bounce the creation onto the UI thread at the one point where the popup
and menu producers are injected into the popup handler, so every caller
stays oblivious to the threading. For a caller that is already on the UI
thread this adds no delay: the main loop drains the enqueued closure in
the same event-processing cycle, before it draws.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A command task streams its output into a view from its own goroutine. To
track soft-wraps (so cursor-positioning escapes from a pager land on the
right line) the write path read the view's live InnerWidth, and the pty
setup read its InnerSize -- both off the UI thread, racing the UI thread
mutating the view's dimensions during layout.
Capture the width on the UI thread instead and hand it to the task: the
escape interpreter keeps a screenColMax it reads from, seeded in NewView
and refreshed per render via View.SetContentWidth (called from
newCmdTask/newPtyTask before the task's goroutine starts), and the pty
size is computed in the after-layout callback rather than in the task's
start func. The view's dimensions stay UI-thread-only; the task uses the
snapshot rather than reading them live.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a task renders different content to a view (a new task key), the
view's scroll origin is reset to the top via onNewKey. That ran on the
task's own goroutine, racing the UI thread, which reads the origin
(OriginY) while laying out and drawing the view -- the single largest
source of view-render data races.
Give ViewBufferManager a bounce primitive (onUIThread) that runs a
function on the UI thread and waits for it, and reset the origin through
it. This is the first use of the primitive; subsequent commits route the
rest of the task's view mutations through it too, so that the view is
only ever touched on the UI thread. It runs as background work
(OnUIThreadAndWaitBackground) so rendering doesn't count towards the app
being busy, matching how the render's gocui task is already created.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The closures that render static content to a main view (newStringTask
and friends) ran on the ViewBufferManager's task goroutine, calling
SetViewContent/SetOrigin/ResetViewOrigin directly on the view. Those
touch view state (the line buffer, hover cells, the origin) that the UI
thread concurrently reads and mutates while laying out and drawing, so
they raced it -- e.g. a string task's SetContent clearing the view's
lines while the UI thread's CopyContent read them, or its SetOrigin
racing the layout's OriginY read.
Bounce the whole closure onto the UI thread instead, so the view is only
touched there. The bounce blocks (OnUIThreadAndWaitBackground) so the
task still completes only once the content has actually been rendered,
which the integration-test idle detection relies on; the background
variant keeps it from counting towards the app being busy, matching the
existing treatment of view rendering as work that must not block a repo
switch.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
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>
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>
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>
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>
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>
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>
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>
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>
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.
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.
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.
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>
The transient contexts (remoteBranches, subCommits, commitFiles) take
over the window of the context they are drilled into from, but until
then they carry a hardcoded initial window ("branches" or
"commits"). Under a gui.sidePanels config where those tabs aren't
their panel's first, no window of that name exists, leaving the
window-to-view map with entries for windows the layout never
produces. The previous commit made such entries harmless, but there's
no reason to have contexts point at nonexistent windows in the first
place; assign them the window hosting branches or commits instead,
which the config validation guarantees to exist.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
With gui.sidePanels, a panel's gocui window is named after its first
tab, so when branches is grouped behind, say, worktrees, there is no
window called "branches" at all. The transient contexts
(remoteBranches, subCommits, commitFiles) initially point at the
windows "branches" and "commits", and layout() showed their views
whenever the window-to-view map named them as their window's current
view — without checking that the window exists in the layout. Since
the map is seeded from the contexts themselves, a window that no
panel owns keeps naming a transient view as its current view, and
that view had just been parked at full screen size (the fallback for
views in unlaid-out windows), so it covered every side panel below it
in z-order.
Only show a transient view if its window actually received dimensions
in this layout.
Fixes#5823.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When the last conflict of a file is resolved, a files refresh both
offers to continue the rebase/merge (if we started it ourselves) and,
via its merge-conflicts scope, escapes from the merge conflicts view
back to the files context. The two race: the prompt is bounced onto
the UI thread by the files worker, while the escape's context push is
queued separately by EscapeMerge, and it deliberately refuses to push
the files context over a popup. So if the prompt opens first, the
escape does nothing, and closing the prompt lands the user in the
stale merge conflicts view — usually already emptied by the escape's
state reset — instead of the files panel. No later refresh rescues
this.
Fix this by escaping from the merge conflicts view right before
opening the prompt. This runs on the UI thread and doesn't hold the
merge conflicts mutex, so it can reset the state and push the files
context synchronously; whichever side runs first, the prompt now
always opens on top of the files context, and EscapeMerge's guarded
push still does nothing only when that's the right thing to do.
This is a timing race with no deterministic regression test; it
showed up as a rare flake in tests that cancel the continue prompt
(e.g. commit/amend_when_there_are_conflicts_and_continue) when
looping the integration tests under the race detector.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Running the integration tests in a loop under the race detector
eventually hung in demo/bisect. The goroutine dump shows the cycle: a
background worker's task.Done() held the task manager's mutex while
blocking on the unbuffered idle-listener channel send, and the test
runner goroutine — the only reader of that channel — was itself blocked
in NewTask on that same mutex, on its way to enqueueing a caption
render (SetCaption -> Render -> OnUIThread). Neither side could
proceed: the notification couldn't be delivered until the test
goroutine got the mutex, and the mutex couldn't be released until the
notification was delivered.
The root problem is that the busy-to-idle notification is a blocking
rendezvous performed while holding the mutex, so it needs the waiter's
cooperation at a moment where the waiter may legitimately need the
mutex first.
Make the notification fire-and-forget instead: WaitUntilIdle waits on a
condition variable and re-checks "is any task busy?" under the mutex,
and the busy-to-idle transition broadcasts, which never blocks. Waiting
is now level-triggered rather than edge-triggered, which is also more
robust: a wait can no longer be satisfied by a stale idle transition
produced by an unrelated background task, because the predicate is
evaluated against the current state. This relies on the previous commit
having made replayed input events carry their task from submission;
without that, the wait could return in the window where an event is in
flight but not yet picked up by the main loop.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
