From be897ce55e5466eec66e703d18abd2b213376188 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 3 Jul 2026 14:01:00 +0200 Subject: [PATCH] Bounce FILES model updates onto the UI thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refreshStateFiles now does its git work on the worker and enqueues a single OnUIThread closure that writes Model.Submodules, Model.Files, and the FileTreeViewModel state together, instead of writing them directly from the worker goroutine. refreshStateSubmoduleConfigs becomes a pure getter (returns the configs; no model write) so the result can be threaded into that same bounce. The STAGING handler wraps RefreshStagingPanel in OnUIThread after fileWg.Wait() so it sees the post-bounce file model rather than the stale pre-refresh one — without this it would race the files bounce queued just above it. Bouncing the write opens a hazard the old synchronous write didn't have: if the user switches repos while this refresh is in flight, the queued closure would fire after resetState has replaced the model with a fresh one for the new repo, silently overwriting it with the previous repo's files. Guard against this with a repo generation: resetState bumps a counter on every switch, refreshStateFiles captures it before its git work, and onUIThreadUnlessRepoChanged drops the bounce if the generation has moved on. This one helper is the general mechanism the remaining scopes' bounces will use too; the same guard covers the rebase-continue prompt, which reads Model.Files right after. A generation counter, not a comparison of the *Model pointer: switching away from and back to a repo reuses that repo's cached state (the same Model pointer), which a pointer comparison would wrongly accept even though the in-flight data is stale. PromptToContinueRebase's Then callback (previous commit) now gets an explanatory comment, since this is the commit that makes it necessary. The explicit locking around these writes (RefreshingFilesMutex in refreshFilesAndSubmodules, FileTreeViewModel.RWMutex around the write in refreshStateFiles) is left in place for now even though it's becoming redundant, to keep this commit focused on the bounce itself; it's removed next. Co-Authored-By: Claude Sonnet 5 --- .../helpers/merge_and_rebase_helper.go | 4 + pkg/gui/controllers/helpers/refresh_helper.go | 84 +++++++++++-------- pkg/gui/gui.go | 15 ++++ pkg/gui/types/common.go | 7 ++ 4 files changed, 77 insertions(+), 33 deletions(-) diff --git a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go index c25416a3b..51488a922 100644 --- a/pkg/gui/controllers/helpers/merge_and_rebase_helper.go +++ b/pkg/gui/controllers/helpers/merge_and_rebase_helper.go @@ -307,6 +307,10 @@ func (self *MergeAndRebaseHelper) PromptToContinueRebase() error { // Need to refresh the files to be really sure if this is the case. // We would otherwise be relying on lazygit's auto-refresh on focus, // but this is not supported by all terminals or on all platforms. + // + // The model.Files update is bounced onto the UI thread, so we have + // to read it in Then; reading it inline here would see the previous + // model. self.c.Refresh(types.RefreshOptions{ Mode: types.SYNC, Scope: []types.RefreshableView{types.FILES}, Then: func() error { diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index 2933d9378..2f2210f49 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -238,7 +238,14 @@ func (self *RefreshHelper) Refresh(options types.RefreshOptions) { if scopeSet.Includes(types.STAGING) { refresh("staging", func() { fileWg.Wait() - self.stagingHelper.RefreshStagingPanel(types.OnFocusOpts{}) + // Bounce onto the UI thread so this runs after the files + // scope's model-update bounce — RefreshStagingPanel reads + // Model.Files (via Files.GetSelected) and would otherwise + // see the pre-refresh model. + self.c.OnUIThread(func() error { + self.stagingHelper.RefreshStagingPanel(types.OnFocusOpts{}) + return nil + }) }) } @@ -667,15 +674,8 @@ func (self *RefreshHelper) refreshTags() error { return nil } -func (self *RefreshHelper) refreshStateSubmoduleConfigs() error { - configs, err := self.c.Git().Submodule.GetConfigs(nil) - if err != nil { - return err - } - - self.c.Model().Submodules = configs - - return nil +func (self *RefreshHelper) refreshStateSubmoduleConfigs() ([]*models.SubmoduleConfig, error) { + return self.c.Git().Submodule.GetConfigs(nil) } // self.refreshStatus is called at the end of this because that's when we can @@ -743,25 +743,40 @@ func (self *RefreshHelper) refreshFilesAndSubmodules(background bool) error { self.c.Mutexes().RefreshingFilesMutex.Lock() defer self.c.Mutexes().RefreshingFilesMutex.Unlock() - if err := self.refreshStateSubmoduleConfigs(); err != nil { + configs, err := self.refreshStateSubmoduleConfigs() + if err != nil { return err } - if err := self.refreshStateFiles(background); err != nil { + if err := self.refreshStateFiles(background, configs); err != nil { return err } - self.c.OnUIThread(func() error { - self.refreshView(self.c.Contexts().Submodules) - self.refreshView(self.c.Contexts().Files) - return nil - }) + self.refreshView(self.c.Contexts().Submodules) + self.refreshView(self.c.Contexts().Files) return nil } -func (self *RefreshHelper) refreshStateFiles(background bool) error { +// onUIThreadUnlessRepoChanged bounces a refresh's model/view update onto the UI +// thread, but drops it if the repo was switched while the refresh was in flight. +// Refresh workers do their git work off the UI thread and enqueue their model +// writes here; a repo switch (which replaces the whole model and context tree) +// bumps the generation, so a write captured under the old generation must not +// clobber the new repo's state. Callers capture the generation with +// State().GetRepoGeneration() before doing their git work and pass it in. +func (self *RefreshHelper) onUIThreadUnlessRepoChanged(generation int, f func() error) { + self.c.OnUIThread(func() error { + if self.c.State().GetRepoGeneration() != generation { + return nil + } + return f() + }) +} + +func (self *RefreshHelper) refreshStateFiles(background bool, submoduleConfigs []*models.SubmoduleConfig) error { fileTreeViewModel := self.c.Contexts().Files.FileTreeViewModel + generation := self.c.State().GetRepoGeneration() prevConflictFileCount := 0 if self.c.UserConfig().Git.AutoStageResolvedConflicts { @@ -822,7 +837,9 @@ func (self *RefreshHelper) refreshStateFiles(background bool) error { // (e.g. in the user's editor). Offer to continue it. We only do this // for operations we started ourselves; prompting for one that was // started outside lazygit (e.g. by a coding agent) would be confusing. - self.c.OnUIThread(func() error { return self.mergeAndRebaseHelper.PromptToContinueRebase() }) + self.onUIThreadUnlessRepoChanged(generation, func() error { + return self.mergeAndRebaseHelper.PromptToContinueRebase() + }) } } else { // Either there's no operation in progress any more, or new conflicts have @@ -835,22 +852,23 @@ func (self *RefreshHelper) refreshStateFiles(background bool) error { }) } - fileTreeViewModel.RWMutex.Lock() - - // only taking over the filter if it hasn't already been set by the user. - if conflictFileCount > 0 && prevConflictFileCount == 0 { - if fileTreeViewModel.GetStatusFilter() == filetree.DisplayAll { - fileTreeViewModel.SetStatusFilter(filetree.DisplayConflicted) - self.c.Contexts().Files.GetView().Subtitle = self.c.Tr.FilterLabelConflictingFiles + self.onUIThreadUnlessRepoChanged(generation, func() error { + // only taking over the filter if it hasn't already been set by the user. + if conflictFileCount > 0 && prevConflictFileCount == 0 { + if fileTreeViewModel.GetStatusFilter() == filetree.DisplayAll { + fileTreeViewModel.SetStatusFilter(filetree.DisplayConflicted) + self.c.Contexts().Files.GetView().Subtitle = self.c.Tr.FilterLabelConflictingFiles + } + } else if conflictFileCount == 0 && fileTreeViewModel.GetStatusFilter() == filetree.DisplayConflicted { + fileTreeViewModel.SetStatusFilter(filetree.DisplayAll) + self.c.Contexts().Files.GetView().Subtitle = "" } - } else if conflictFileCount == 0 && fileTreeViewModel.GetStatusFilter() == filetree.DisplayConflicted { - fileTreeViewModel.SetStatusFilter(filetree.DisplayAll) - self.c.Contexts().Files.GetView().Subtitle = "" - } - self.c.Model().Files = files - fileTreeViewModel.SetTree() - fileTreeViewModel.RWMutex.Unlock() + self.c.Model().Submodules = submoduleConfigs + self.c.Model().Files = files + fileTreeViewModel.SetTree() + return nil + }) return nil } diff --git a/pkg/gui/gui.go b/pkg/gui/gui.go index 87a87f7b2..834cf1e2d 100644 --- a/pkg/gui/gui.go +++ b/pkg/gui/gui.go @@ -12,6 +12,7 @@ import ( "sort" "strings" "sync" + "sync/atomic" "time" "github.com/jesseduffield/lazycore/pkg/boxlayout" @@ -111,6 +112,11 @@ type Gui struct { PopupHandler types.IPopupHandler + // Bumped every time we switch to a different repository (in resetState). + // Used to drop refresh results that were computed for a repo we've since + // navigated away from. See RefreshHelper.onUIThreadUnlessRepoChanged. + repoGeneration atomic.Int32 + // we use this to decide whether we'll return to the original directory that // lazygit was opened in, or if we'll retain the one we're currently in. RetainOriginalDir bool @@ -169,6 +175,10 @@ func (self *StateAccessor) GetRepoState() types.IRepoStateAccessor { return self.gui.State } +func (self *StateAccessor) GetRepoGeneration() int { + return int(self.gui.repoGeneration.Load()) +} + func (self *StateAccessor) GetPagerConfig() *config.PagerConfig { return self.gui.pagerConfig } @@ -575,6 +585,11 @@ func (gui *Gui) checkForChangedConfigsThatDontAutoReload(oldConfig *config.UserC // resetState reuses the repo state from our repo state map, if the repo was // open before; otherwise it creates a new one. func (gui *Gui) resetState(startArgs appTypes.StartArgs) types.Context { + // Bump the repo generation so that any refresh still in flight for the + // previous repo drops its model update instead of applying it here (see + // RefreshHelper.onUIThreadUnlessRepoChanged). + gui.repoGeneration.Add(1) + // Un-highlight the current view if there is one. The reason we do this is // that the repo we are switching to might have a different view focused, // and would then show an inactive highlight for the previous view. diff --git a/pkg/gui/types/common.go b/pkg/gui/types/common.go index ad28972ae..7fc75aee0 100644 --- a/pkg/gui/types/common.go +++ b/pkg/gui/types/common.go @@ -384,6 +384,13 @@ type IStateAccessor interface { GetItemOperation(item HasUrn) ItemOperation SetItemOperation(item HasUrn, operation ItemOperation) ClearItemOperation(item HasUrn) + + // A counter that is bumped every time we switch to a different repository + // (see Gui.resetState). Refresh workers capture it before doing their git + // work and pass it to onUIThreadUnlessRepoChanged, so that a model update + // computed for one repo can be dropped rather than applied to another if the + // user switched repos while the refresh was in flight. + GetRepoGeneration() int } type IRepoStateAccessor interface {