It never changes inside this function, so there's no need to recompute
it with every loop iteration.
Equivalent to the change that was made to isDescendentOfSelectedNodes in
files_controller.go in d0c6e27fee.
When lazygit is running in a VS Code window that doesn't have the focus
(e.g. a split tab) and you click in the commits panel to focus it and
select a commit at the same time, it would briefly select the commit you
clicked but then flash back to the commit that was previously selected.
This PR fixes that so that the clicked commit stays selected.
This was only a problem with VS Code; in Zed's builtin terminal it
worked, apparently because it first dispatches the click and then the
focus-in event. Ghostty and iTerm2 were not affected because they don't
dispatch clicks in inactive windows or tabs at all.
Labelling as ignore-for-release because it fixes a regression that was
introduced since the last release.
This fixes the problem described in the previous commit; we no longer
capture the selection at the start of the refresh. There's no reason to
do that (we don't do it for branches either). It is enough to capture
the selection in the final bounce, before we assign the new model slice.
When clicking in the commits view of lazygit running in an unfocused VS
Code window, VS Code first sends us the focus-in event and then the
mouse-click. The focus-in refresh captures the selection when it starts,
then we handle the mouse click and you briefly see the clicked row
getting selected, but then the selection flashes back to the original
row as the refresh restores it when done.
CreatePseudoConsole and ResizePseudoConsole reject zero dimensions with
E_INVALIDARG, but we legitimately request them: the pty is sized after
the main view, and that view is zero-sized while hidden, e.g. in
full-screen mode with a side panel focused. Entering that mode while a
custom pager is configured therefore made StartPty fail (degrading to
unpaged output now that the fallback works), and resizing a live pty
from onResize would fail layout. The Unix pty accepts zero sizes, so
the clamp lives in the Windows implementation only.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CreatePseudoConsole rejects zero dimensions with E_INVALIDARG, so
starting a pty sized after a hidden (and thus zero-sized) view fails.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
NewCmdTask feeds the reader returned by its start func into a
bufio.Scanner, and Scanner.Scan panics with a nil pointer dereference
when that reader is nil. Two start funcs could produce one:
- newPtyTask's fallback for a failed StartPty returned a literal nil
reader, alongside an ExecCmd that was never started, so the intended
"fall back to a plain cmd task" never worked. This crashed lazygit on
Windows when using a custom pager with the main view zero-sized, e.g.
after pressing + twice to enter full-screen mode with a side panel
focused: ConPTY rejects zero dimensions, making StartPty fail.
- startCmdWithPipe returned nil when the pipe couldn't be created,
which the Unix pty fallback path can trigger, since a failed pty
start can leave the tty assigned to the command's stdout.
Make startCmdWithPipe never return a nil reader: when the pipe can't be
created, don't start the command at all and return an empty reader so
the task shuts down cleanly with the error in the log. Then route
newPtyTask's fallback through it, so a StartPty failure degrades to
running the command without a pty: the pager is lost, but the command's
output still renders.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
NewCmdTask feeds the reader returned by its start func straight into a
bufio.Scanner, whose Scan panics on a nil reader with a nil pointer
dereference. startCmdWithPipe returns exactly that when the pipe cannot
be created.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The fallback path in newPtyTask (taken when StartPty fails) needs the
same start-the-command-with-a-pipe logic that newCmdTask uses, so pull
it out into a helper that both can share. No behavior change.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A refresh from the UI thread returns immediately and applies its model
and view updates as queued UI-thread callbacks. A key pressed before
those have run is handled against the stale, pre-refresh state. For most
keys that's harmless, but some handlers turn that state into git
commands: pressing space twice in quick succession in the staging panel
builds the second patch from the already-applied diff and fails with
'patch does not apply', because the refresh after the first press is
what moves the selection to the next stageable hunk.
Notably, this is not just a regression of the recent change that made
UI-thread refreshes non-blocking; the window was merely much narrower
before. A blocking refresh parked the UI thread while the scopes'
bounces were queued, and the event loop drains pending keyboard input
with priority over queued user events, so a key pressed during the
blocked window still beat the queued state updates. The guarantee that
the next keypress sees post-refresh state had already ended when the
scopes' state updates moved from worker-side mutex-guarded writes to
UI-thread bounces.
Fix it with the input-blocking mechanism we already use for commit
surgery, exposed as a new RefreshBlockingInput entry point: it begins
blocking events synchronously in the calling handler, and ends the block
from a callback that the finishing step queues behind the refresh's own
updates. Keys pressed while the refresh is in flight are buffered and
replayed, in order, against the fully refreshed state; since a replayed
key's handler re-enters this same path, a burst of keypresses applies
sequentially, each one seeing the previous one's refresh. Unlike the old
blocking refreshes, this doesn't freeze the UI thread: rendering,
spinners, resizing, and mouse scrolling keep working while input is
withheld.
Blocking input is opt-in per call site rather than the default for all
UI-thread refreshes, because most refreshes (the focus-in and startup
refreshes, say) don't produce state that the next keypress depends on,
and blocking on them would delay typing for no reason. It should also be
limited to quick, narrow-scoped refreshes: a full refresh, or any scope
that pulls in COMMITS, can take very long in large repos and should
usually not hold up input.
Use the new function for the staging panel's stage/discard/edit-hunk
refreshes, for moving rebase todos, and for stash operations.
Labelling as ignore-for-release because it fixes regressions introduced
after the last release.
On startup we don't want to block input during the initial refresh (it
should be possible to press, say, `4` to jump to the commits panel right
after startup without a delay), and we also want panels to show their
contents as soon as possible; it doesn't matter so much that it's not in
sync, we go from empty to populated here. However, when switching repos
it can be confusing that some panels that are slow to update still show
the old repo's data while others already show the new one's data, so
update the UI only when everything is ready, and also block input to
prevent accidentally trying to act on the old, stale data.
Popping or dropping a stash shifts the indices of the entries below it,
and renaming re-creates the stash at the top, shifting all the others.
The stash model is only rebuilt by the refresh, which finishes in the
background, so acting on the next entry in quick succession — pressing
the key, confirming the popup, and pressing again right away — reads the
stale pre-operation indices and targets the wrong stash. Note that the
confirmation popup is no protection here: the race starts when the
confirm handler runs, and the next keypress can easily beat the refresh.
Use RefreshBlockingInput so a quick follow-up keypress is buffered and
replayed once the refreshed stash list is in place.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
Moving a todo up or down rewrites the todo file and advances the
selection synchronously, but the commits model is only rebuilt by the
refresh, which finishes in the background. A second keypress arriving
before that reads the pre-move model at the advanced selection index —
that's the todo the first move swapped with, so the second press moves
that one back instead of moving the selected todo further. Two rapid
presses (e.g. from holding the key down) thus amount to a net no-op.
The two presses also spawn two racing refreshes whose model updates can
land in either order, so the todo list can even end up disagreeing with
the todo file. That's why the test continues the rebase and asserts the
resulting commit order instead of the displayed list: the rebase replays
the file, which is deterministic.
The test documents this currently broken behavior; the fix comes next.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A refresh from the UI thread returns immediately and applies its model
and view updates as queued UI-thread callbacks. A key pressed before
those have run is handled against the stale, pre-refresh state. For most
keys that's harmless, but some handlers turn that state into git
commands: pressing space twice in quick succession in the staging panel
builds the second patch from the already-applied diff and fails with
'patch does not apply', because the refresh after the first press is
what moves the selection to the next stageable hunk.
Notably, this is not just a regression of the recent change that made
UI-thread refreshes non-blocking; the window was merely much narrower
before. A blocking refresh parked the UI thread while the scopes'
bounces were queued, and the event loop drains pending keyboard input
with priority over queued user events, so a key pressed during the
blocked window still beat the queued state updates. The guarantee that
the next keypress sees post-refresh state had already ended when the
scopes' state updates moved from worker-side mutex-guarded writes to
UI-thread bounces.
Fix it with the input-blocking mechanism we already use for commit
surgery, exposed as a new RefreshBlockingInput entry point: it begins
blocking events synchronously in the calling handler, and ends the
block from a callback that the finishing step queues behind the
refresh's own updates. Keys pressed while the refresh is in flight are
buffered and replayed, in order, against the fully refreshed state;
since a replayed key's handler re-enters this same path, a burst of
keypresses applies sequentially, each one seeing the previous one's
refresh. Unlike the old blocking refreshes, this doesn't freeze the UI
thread: rendering, spinners, resizing, and mouse scrolling keep working
while input is withheld.
Blocking input is opt-in per call site rather than the default for all
UI-thread refreshes, because most refreshes (the focus-in and startup
refreshes, say) don't produce state that the next keypress depends on,
and blocking on them would delay typing for no reason. It should also be
limited to quick, narrow-scoped refreshes: a full refresh, or any scope
that pulls in COMMITS, can take very long in large repos and should
usually not hold up input.
The staging panel's stage/discard/edit-hunk refreshes use it now.
Pressing space twice in quick succession in the staging panel is supposed
to stage two hunks: the refresh triggered by the first press rebuilds the
panel's diff and moves the selection to the next stageable hunk, and the
second press stages that.
Since we made UI-thread refreshes non-blocking, the second press is
handled as soon as it arrives, while that refresh is still in flight. It
then reads the stale pre-refresh diff, builds the first hunk's patch
again, and git apply fails with 'patch does not apply' because those
lines are already in the index.
The test documents this currently broken behavior; the fix comes next.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The test driver waits for lazygit to become idle after every keypress, so
tests could never exercise what happens when a key arrives while the
previous key's processing is still in flight — for example while the
refresh triggered by the previous key hasn't updated the model yet. Real
users type faster than that all the time.
PressRapidly injects all its keys back to back and waits for idle only
once at the end, so the second and later keys are queued before the first
one's processing has finished. The next commit uses this to demonstrate a
bug in exactly that scenario.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
Make the background fetch and the refresh that runs after it more safe
against racing with a concurrent foreground repo switch (i.e. switching
worktrees, repos, or submodules). This fixes a bunch of different
problems; see the individual commit messages for details.
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 cached git config runs its `git config` reads through raw
exec.Command calls, outside the pinned git command builder, so they
followed the process working directory. A cache miss on a stale
instance — one still in use by a refresh that crossed a repo switch —
would therefore read the new repo's local config while computing data
for the old one. Give the cache a directory, set once by NewGitCommand
right after it determines the repo paths (the object is created fresh
for every repo switch, so no cross-repo cache invalidation is needed),
and run every config command there.
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>
Lazygit changes the process working directory when switching repos, but
work that is still in flight for the previous repo can keep spawning
git commands after the switch — most notably a background refresh. Its
model writes are already dropped by the repo generation guard, but its
git commands would now run against the new repo. That is wasted work at
best; at worst it surfaces spurious error popups (the behind-base-
branch computation failing with "no such ref" when the old repo's main
branch doesn't exist in the new one) and pollutes caches belonging to
the old repo's reusable state (e.g. MainBranches' existing-branches
cache), which the user sees when switching back.
Give the git command builder the directory of the repo it was created
for, and pin every command it produces to that directory. The pinned
directory and the process cwd are identical until a switch happens
(NewGitCommand chdirs to the worktree path right before creating the
builder), so nothing changes in the steady state; the pin only takes
effect for commands built through a previous repo's GitCommand instance
after a switch, which now keep addressing the repo they were built for.
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>
Deleting a nested submodule (and updating its URL) chdir'd the whole
process into the parent module, ran its git commands there, and chdir'd
back. Only those commands need to run there, and a process-wide chdir
leaks the parent module's directory into any command another goroutine
spawns during that window (e.g. a background refresh's). Set the
directory on the commands themselves instead.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
To spot slow or anomalous tests across CI runs, record each test's run
duration when LAZYGIT_TEST_TIMING is set (to a file path);
run_integration_tests.sh prints them at the end, sorted by slowest
first. CI sets it for all integration jobs.
The harness appends to a file rather than writing to stdout/stderr
because `go test` captures those and only surfaces them with -v, which
would drown the signal in every test's verbose logs.
Co-Authored-By: Claude Opus 4.8 (1M context) <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>
Add one extra integration-tests job that runs the whole suite under the
race detector. A `race` matrix dimension (default false) plus an include
entry adds a single git-latest job with LAZYGIT_RACE_DETECTOR set; races
live in lazygit's own Go code rather than in git, so one git version is
enough, and using latest skips the git-build steps.
The race job skips coverage collection: it's redundant with the non-race
latest job and would only slow the -race build down further.
Go's default 10-minute timeout was enough for running integration tests
normally (both locally and on CI), but with race detection turned on
they can take much longer to run. Increase the timeout unconditionally
to 30 minutes; we don't bother making a distinction between race vs.
normal, because a longer timeout doesn't hurt (I can't recall having hit
the global timeout ever; and we still have the per-test watchdog that
kills an individual test after 40s).
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>
When lazygit exits but leaves behind a subprocess that inherited its
stderr pipe and has detached from the pty, cmd.Wait() blocks in
awaitGoroutines waiting for that pipe to reach EOF -- which never
happens while the straggler is alive. With no WaitDelay set, that wait
is unbounded, so a single leaked process hangs the whole test binary
until the 10-minute global timeout fires and panics. Worse, the timeout
discards whatever lazygit wrote to stderr before exiting (a panic, a
-race report), which is exactly the output needed to diagnose the
failure.
This surfaces under -race, where lazygit runs slow enough to widen the
window for a spawned command to still be alive when lazygit quits, and
it's a blocker for enabling the race detector on CI.
Bound the wait with cmd.WaitDelay so Wait force-closes the pipe and
returns ErrWaitDelay instead of hanging, surface the captured stderr as
the error (falling back to the wait error when nothing was printed), and
kill the child's process group on failure so a straggler can't linger
into a later test or pile up across a run.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When pressing ctrl+z to suspend lazygit, and then `fg` to bring it to
the foreground again, sometimes it wouldn't come to the foreground,
stalling in the background with one core using 100% CPU.
Fixes#5309.
The previous two commits stop gocui from flushing while suspended, but
that guard cannot be fully airtight from gocui's side: it is a
check-then-act on the suspended flag, so a flush racing the suspend
itself (e.g. from a spinner goroutine) could still reach the screen
just as it disengages, and tcell's disengageFinish mutates the cell
buffer without holding the screen lock. Upstream now closes this at
the source (gdamore/tcell#1139): draw() returns immediately on a
disengaged screen, and the draw scan loop can no longer stall on the
width-0 cells that a released cell buffer reports (#5309).
The delta over the previously pinned snapshot is these two fixes, a
CSI R input decode fix, a wasm packaging chore, and dependency bumps.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Until now the repaint after fg was accidental: it only happened because
the suspend keybinding handler still had a flush pending on the UI
thread, and only if that flush happened to run after the SIGCONT
handler had re-engaged the screen. Now that flushes are skipped while
suspended, losing that race would leave the screen blank until the next
input event arrives, so schedule a redraw explicitly (#5309).
When suspending with ctrl+z, the suspend keybinding handler disengages
the screen and then sends SIGSTOP to the process group, so the UI
thread freezes at the return from kill(2) with the handler's follow-up
flush still pending. When fg continues the process, that pending flush
races the SIGCONT handler's Resume. If the flush wins, Show() draws
against the disengaged screen, whose cell buffer tcell has released to
0x0 while its width/height still hold the old size; drawCell() then
reports width 0 for the out-of-range cell, the draw loop's
'x += width - 1' never advances, and the UI thread spins forever while
holding the tcell screen lock. Resume in turn blocks forever on that
lock, so the screen never re-engages and no input is ever read again:
the hard stall of #5309, only recoverable by killing the process.
Guard both flush paths with the suspended flag. For the flag to
guarantee that the screen is engaged whenever it is false, Resume must
clear it only after re-engaging (it used to clear it before); Suspend
already sets it before disengaging. This also covers the pre-existing
unsynchronized suspended check in draw(), which is subsumed by the
guards and can go.
The regression test cannot use the demonstrate-then-fix pattern: on
unfixed code the flush goroutine spins holding the screen lock, which
deadlocks any subsequent screen call including the test cleanup's
Close().
When lazygit is suspended with ctrl+z and brought back with fg, nothing
deliberately triggers a redraw. The screen only repaints because the UI
thread happens to have a flush pending from the suspend keybinding, and
that flush races the SIGCONT handler's resume; when it loses in the
right way, the terminal shows a blank screen until the next input event
arrives (#5309).
Grep-based navigation needs manual filtering for the many colliding
method names in this codebase, while gopls answers reference and
implementation questions type-aware and exactly. Scope the guidance to
the symbol tools and keep grep for textual searches: gopls' own MCP
instructions prescribe running vulncheck at session start and
go_file_context after every file read, which costs more than it helps
here. The server is registered per user and machine, so sessions
without it must just fall back to grep rather than try to set it up.
