diff --git a/pkg/gui/controllers/helpers/inline_status_helper.go b/pkg/gui/controllers/helpers/inline_status_helper.go index f2ca7ec17..814a11406 100644 --- a/pkg/gui/controllers/helpers/inline_status_helper.go +++ b/pkg/gui/controllers/helpers/inline_status_helper.go @@ -139,16 +139,17 @@ func (self *InlineStatusHelper) stop(opts InlineStatusOpts) { self.c.State().ClearItemOperation(opts.Item) // Re-render the context to remove the inline status now that the operation - // finished. Any refresh it triggered must be synchronous, not async: by the - // time we get here a synchronous refresh has already updated the model and - // queued its own re-render, and since UI-thread callbacks run in order, the - // render we queue here runs after it and draws the up-to-date model without - // the inline status. An async refresh might not have updated the model yet, - // so this render could briefly show the stale, pre-operation model: when - // pushing a branch, for example, it would flash the old ↑3↓7 ahead/behind - // counts for a moment before the refresh replaced them with a green - // checkmark. (Operations that don't refresh at all are fine too: there's - // nothing stale to show, so this just drops the status.) + // finished. The operation must trigger its refresh via RefreshFromWorker + // before we get here: that call returns only once the refresh's model + // updates have been enqueued on the UI thread, and since UI-thread + // callbacks run in order, the render we queue here runs after them and + // draws the up-to-date model without the inline status. A refresh whose + // model updates aren't enqueued yet by this point would make this render + // briefly show the stale, pre-operation model: when pushing a branch, for + // example, it would flash the old ↑3↓7 ahead/behind counts for a moment + // before the refresh replaced them with a green checkmark. (Operations + // that don't refresh at all are fine too: there's nothing stale to show, + // so this just drops the status.) self.renderContext(opts.ContextKey) } diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index 8ae62d9dd..a85c58cf2 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -1216,11 +1216,11 @@ func (self *RefreshHelper) onUIThread(background bool, f func() error) { // runs on the UI thread (calledFromWorker is false) fn runs inline; when it runs // on a worker, fn is dispatched to the UI thread and we block for it. // -// The inline case matters for correctness as much as the hop: a SYNC refresh -// initiated on the UI thread parks that thread in a wg.Wait while its scope -// workers run, so a scope worker that tried to hop to the UI thread there would -// deadlock. Capturing before those workers are spawned — inline, on the UI -// thread — avoids that entirely. +// The inline case matters for correctness as much as the hop: OnUIThreadAndWait +// must not be called from the UI thread itself (it would park the thread +// waiting for a callback that only it can run), and capturing inline also +// guarantees the snapshot reflects the state at the moment Refresh was called, +// before the calling handler regains control and can mutate it. func (self *RefreshHelper) captureOnUIThread(calledFromWorker bool, background bool, fn func()) { if !calledFromWorker { fn() diff --git a/pkg/gui/controllers/remotes_controller.go b/pkg/gui/controllers/remotes_controller.go index 76bd16bb1..cd05e5ff9 100644 --- a/pkg/gui/controllers/remotes_controller.go +++ b/pkg/gui/controllers/remotes_controller.go @@ -159,8 +159,7 @@ func (self *RemotesController) addAndCheckoutRemote(remoteName string, remoteUrl // Refresh the remotes so that we can select the new one. The remotes model // update is bounced onto the UI thread, so the selection (which reads // Model.Remotes) has to run in Then; reading it inline here would see the - // previous model. Loading remotes is not expensive, so a sync refresh is - // affordable. + // previous model. self.c.Refresh(types.RefreshOptions{ Scope: []types.RefreshableView{types.REMOTES}, Then: func() error {