Move Github PR cache out of state.yml into a separate file

Having the cache in state.yml causes this file to be rewritten every
60s, which is annoying if you have a lazygit running in the background
somewhere without even realizing it, and it keeps overwriting the
foreground lazygit's newer command shell history and recent repos list
with its stale data. State.yml should only contain things that change in
response to user actions, not periodically.

pkg/config/app_config.go

@@ -21,17 +21,18 @@ import (
 
 // AppConfig contains the base configuration fields required for lazygit.
 type AppConfig struct {
-	debug                 bool   `long:"debug" env:"DEBUG" default:"false"`
-	version               string `long:"version" env:"VERSION" default:"unversioned"`
-	buildDate             string `long:"build-date" env:"BUILD_DATE"`
-	name                  string `long:"name" env:"NAME" default:"lazygit"`
-	buildSource           string `long:"build-source" env:"BUILD_SOURCE" default:""`
-	userConfig            *UserConfig
-	globalUserConfigFiles []*ConfigFile
-	userConfigFiles       []*ConfigFile
-	userConfigDir         string
-	tempDir               string
-	appState              *AppState
+	debug                  bool   `long:"debug" env:"DEBUG" default:"false"`
+	version                string `long:"version" env:"VERSION" default:"unversioned"`
+	buildDate              string `long:"build-date" env:"BUILD_DATE"`
+	name                   string `long:"name" env:"NAME" default:"lazygit"`
+	buildSource            string `long:"build-source" env:"BUILD_SOURCE" default:""`
+	userConfig             *UserConfig
+	globalUserConfigFiles  []*ConfigFile
+	userConfigFiles        []*ConfigFile
+	userConfigDir          string
+	tempDir                string
+	appState               *AppState
+	githubPullRequestCache *githubPullRequestCache
 }
 
 type AppConfigurer interface {
@@ -51,6 +52,8 @@ type AppConfigurer interface {
 
 	GetAppState() *AppState
 	SaveAppState() error
+	GetCachedGithubPullRequests(repoPath string) ([]CachedPullRequest, error)
+	SaveCachedGithubPullRequests(repoPath string, pullRequests []CachedPullRequest) error
 }
 
 type ConfigFilePolicy int
@@ -107,19 +110,21 @@ func NewAppConfig(
 	if err != nil {
 		return nil, err
 	}
+	githubPullRequestCache := loadGithubPullRequestCache()
 
 	appConfig := &AppConfig{
-		name:                  name,
-		version:               version,
-		buildDate:             date,
-		debug:                 debuggingFlag,
-		buildSource:           buildSource,
-		userConfig:            userConfig,
-		globalUserConfigFiles: configFiles,
-		userConfigFiles:       configFiles,
-		userConfigDir:         configDir,
-		tempDir:               tempDir,
-		appState:              appState,
+		name:                   name,
+		version:                version,
+		buildDate:              date,
+		debug:                  debuggingFlag,
+		buildSource:            buildSource,
+		userConfig:             userConfig,
+		globalUserConfigFiles:  configFiles,
+		userConfigFiles:        configFiles,
+		userConfigDir:          configDir,
+		tempDir:                tempDir,
+		appState:               appState,
+		githubPullRequestCache: githubPullRequestCache,
 	}
 
 	return appConfig, nil
@@ -666,6 +671,20 @@ func (c *AppConfig) GetAppState() *AppState {
 	return c.appState
 }
 
+func (c *AppConfig) GetCachedGithubPullRequests(repoPath string) ([]CachedPullRequest, error) {
+	if c.githubPullRequestCache == nil {
+		return nil, nil
+	}
+	return c.githubPullRequestCache.get(repoPath), c.githubPullRequestCache.takeLoadError()
+}
+
+func (c *AppConfig) SaveCachedGithubPullRequests(repoPath string, pullRequests []CachedPullRequest) error {
+	if c.githubPullRequestCache == nil {
+		return nil
+	}
+	return c.githubPullRequestCache.save(repoPath, pullRequests)
+}
+
 func (c *AppConfig) GetUserConfigPaths() []string {
 	return lo.FilterMap(c.userConfigFiles, func(f *ConfigFile, _ int) (string, bool) {
 		return f.Path, f.exists
@@ -837,28 +856,10 @@ type AppState struct {
 	ShellCommandsHistory []string `yaml:"customcommandshistory"`
 
 	HideCommandLog bool
-
-	// Cache of GitHub pull requests per repo path, so that PR info can be
-	// shown instantly on startup before the async refresh completes.
-	GithubPullRequests map[string][]CachedPullRequest `yaml:"githubPullRequests"`
-}
-
-// CachedPullRequest stores the essential fields of a GitHub pull request
-// for persisting in the app state cache.
-type CachedPullRequest struct {
-	HeadRefName         string `yaml:"headRefName"`
-	Number              int    `yaml:"number"`
-	Title               string `yaml:"title"`
-	State               string `yaml:"state"`
-	ChecksState         string `yaml:"checksState,omitempty"`
-	Url                 string `yaml:"url"`
-	HeadRepositoryOwner string `yaml:"headRepositoryOwner"`
 }
 
 func getDefaultAppState() *AppState {
-	return &AppState{
-		GithubPullRequests: make(map[string][]CachedPullRequest),
-	}
+	return &AppState{}
 }
 
 func LogPath() (string, error) {

pkg/config/dummies.go

@@ -9,11 +9,12 @@ func NewDummyAppConfig() *AppConfig {
 	userConfig := GetDefaultConfig()
 	userConfig.Keybinding.MergeLegacyAltKeybindings()
 	appConfig := &AppConfig{
-		name:       "lazygit",
-		version:    "unversioned",
-		debug:      false,
-		userConfig: userConfig,
-		appState:   &AppState{},
+		name:                   "lazygit",
+		version:                "unversioned",
+		debug:                  false,
+		userConfig:             userConfig,
+		appState:               &AppState{},
+		githubPullRequestCache: newGithubPullRequestCache(""),
 	}
 	_ = yaml.Unmarshal([]byte{}, appConfig.appState)
 	return appConfig

pkg/config/github_pull_request_cache.go

@@ -0,0 +1,124 @@
+package config
+
+import (
+	"encoding/json"
+	"fmt"
+	"os"
+	"path/filepath"
+	"sync"
+)
+
+const githubPullRequestsCacheFileName = "github_pull_requests.json"
+
+// CachedPullRequest stores the essential fields of a GitHub pull request.
+type CachedPullRequest struct {
+	HeadRefName         string `json:"headRefName"`
+	Number              int    `json:"number"`
+	Title               string `json:"title"`
+	State               string `json:"state"`
+	ChecksState         string `json:"checksState,omitempty"`
+	Url                 string `json:"url"`
+	HeadRepositoryOwner string `json:"headRepositoryOwner"`
+}
+
+type githubPullRequestCache struct {
+	mutex                  sync.Mutex
+	path                   string
+	pullRequestsByRepoPath map[string][]CachedPullRequest
+	loadErr                error
+}
+
+func loadGithubPullRequestCache() *githubPullRequestCache {
+	path, err := githubPullRequestCachePath()
+	if err != nil {
+		cache := newGithubPullRequestCache("")
+		cache.loadErr = err
+		return cache
+	}
+
+	cache := newGithubPullRequestCache(path)
+	cache.load()
+	return cache
+}
+
+func githubPullRequestCachePath() (string, error) {
+	path, err := stateFilePath(stateFileName)
+	if err != nil {
+		return "", err
+	}
+
+	return filepath.Join(filepath.Dir(path), githubPullRequestsCacheFileName), nil
+}
+
+func newGithubPullRequestCache(path string) *githubPullRequestCache {
+	return &githubPullRequestCache{
+		path:                   path,
+		pullRequestsByRepoPath: make(map[string][]CachedPullRequest),
+	}
+}
+
+func (c *githubPullRequestCache) load() {
+	if c.path == "" {
+		return
+	}
+
+	content, err := os.ReadFile(c.path)
+	if err != nil {
+		if !os.IsNotExist(err) {
+			c.loadErr = fmt.Errorf("reading GitHub pull request cache: %w", err)
+		}
+		return
+	}
+	if len(content) == 0 {
+		return
+	}
+
+	if err := json.Unmarshal(content, &c.pullRequestsByRepoPath); err != nil {
+		c.pullRequestsByRepoPath = make(map[string][]CachedPullRequest)
+		c.loadErr = fmt.Errorf("parsing GitHub pull request cache: %w", err)
+	} else if c.pullRequestsByRepoPath == nil {
+		c.pullRequestsByRepoPath = make(map[string][]CachedPullRequest)
+	}
+}
+
+func (c *githubPullRequestCache) get(repoPath string) []CachedPullRequest {
+	c.mutex.Lock()
+	defer c.mutex.Unlock()
+
+	return append([]CachedPullRequest(nil), c.pullRequestsByRepoPath[repoPath]...)
+}
+
+// takeLoadError returns the error, if any, that occurred while loading the
+// cache from disk, clearing it so that it is reported only once.
+func (c *githubPullRequestCache) takeLoadError() error {
+	c.mutex.Lock()
+	defer c.mutex.Unlock()
+
+	loadErr := c.loadErr
+	c.loadErr = nil
+	return loadErr
+}
+
+func (c *githubPullRequestCache) save(repoPath string, pullRequests []CachedPullRequest) error {
+	c.mutex.Lock()
+	defer c.mutex.Unlock()
+
+	c.pullRequestsByRepoPath[repoPath] = append([]CachedPullRequest(nil), pullRequests...)
+	if c.path == "" {
+		return nil
+	}
+
+	content, err := json.MarshalIndent(c.pullRequestsByRepoPath, "", "  ")
+	if err != nil {
+		return err
+	}
+	content = append(content, '\n')
+
+	// Apparently when people have read-only permissions they prefer us to fail
+	// silently, so don't propagate permission errors.
+	if err := os.WriteFile(c.path, content, 0o644); err != nil && !os.IsPermission(err) {
+		return err
+	}
+
+	return nil
+}

pkg/config/github_pull_request_cache_test.go

@@ -0,0 +1,141 @@
+package config
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"sync"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGithubPullRequestCachePath(t *testing.T) {
+	stateDir := t.TempDir()
+	t.Setenv("CONFIG_DIR", stateDir)
+
+	path, err := githubPullRequestCachePath()
+
+	assert.NoError(t, err)
+	assert.Equal(t, filepath.Join(stateDir, githubPullRequestsCacheFileName), path)
+}
+
+func TestGithubPullRequestCache(t *testing.T) {
+	path := filepath.Join(t.TempDir(), githubPullRequestsCacheFileName)
+	cache := newGithubPullRequestCache(path)
+	repoOnePullRequests := []CachedPullRequest{{
+		HeadRefName:         "first-branch",
+		Number:              1,
+		Title:               "First pull request",
+		State:               "OPEN",
+		Url:                 "https://github.com/owner/repo/pull/1",
+		HeadRepositoryOwner: "owner",
+	}}
+	repoTwoPullRequests := []CachedPullRequest{{
+		HeadRefName:         "second-branch",
+		Number:              2,
+		Title:               "Second pull request",
+		State:               "MERGED",
+		Url:                 "https://github.com/other/repo/pull/2",
+		HeadRepositoryOwner: "other",
+	}}
+
+	assert.NoError(t, cache.save("/repo/one", repoOnePullRequests))
+	assert.NoError(t, cache.save("/repo/two", repoTwoPullRequests))
+
+	content, err := os.ReadFile(path)
+	assert.NoError(t, err)
+	assert.Equal(t, `{
+  "/repo/one": [
+    {
+      "headRefName": "first-branch",
+      "number": 1,
+      "title": "First pull request",
+      "state": "OPEN",
+      "url": "https://github.com/owner/repo/pull/1",
+      "headRepositoryOwner": "owner"
+    }
+  ],
+  "/repo/two": [
+    {
+      "headRefName": "second-branch",
+      "number": 2,
+      "title": "Second pull request",
+      "state": "MERGED",
+      "url": "https://github.com/other/repo/pull/2",
+      "headRepositoryOwner": "other"
+    }
+  ]
+}
+`, string(content))
+
+	reloadedCache := newGithubPullRequestCache(path)
+	reloadedCache.load()
+	assert.Equal(t, repoOnePullRequests, reloadedCache.get("/repo/one"))
+	assert.Equal(t, repoTwoPullRequests, reloadedCache.get("/repo/two"))
+	assert.NoError(t, reloadedCache.takeLoadError())
+}
+
+func TestGithubPullRequestCacheIgnoresMalformedContent(t *testing.T) {
+	path := filepath.Join(t.TempDir(), githubPullRequestsCacheFileName)
+	assert.NoError(t, os.WriteFile(path, []byte("{"), 0o644))
+
+	cache := newGithubPullRequestCache(path)
+	cache.load()
+
+	assert.ErrorContains(t, cache.takeLoadError(), "parsing GitHub pull request cache")
+	assert.Empty(t, cache.get("/repo"))
+	assert.NoError(t, cache.save("/repo", []CachedPullRequest{{Number: 1}}))
+
+	reloadedCache := newGithubPullRequestCache(path)
+	reloadedCache.load()
+	assert.Equal(t, []CachedPullRequest{{Number: 1}}, reloadedCache.get("/repo"))
+	assert.NoError(t, reloadedCache.takeLoadError())
+}
+
+func TestGithubPullRequestCacheDoesNotModifyAppState(t *testing.T) {
+	stateDir := t.TempDir()
+	t.Setenv("CONFIG_DIR", stateDir)
+	statePath := filepath.Join(stateDir, stateFileName)
+	stateContent := []byte("recentrepos:\n  - /repo\n")
+	assert.NoError(t, os.WriteFile(statePath, stateContent, 0o644))
+
+	cache := loadGithubPullRequestCache()
+	assert.NoError(t, cache.save("/repo", []CachedPullRequest{{Number: 1}}))
+
+	actualStateContent, err := os.ReadFile(statePath)
+	assert.NoError(t, err)
+	assert.Equal(t, stateContent, actualStateContent)
+	_, err = os.Stat(filepath.Join(stateDir, githubPullRequestsCacheFileName))
+	assert.NoError(t, err)
+}
+
+func TestGithubPullRequestCacheSerializesConcurrentSaves(t *testing.T) {
+	path := filepath.Join(t.TempDir(), githubPullRequestsCacheFileName)
+	cache := newGithubPullRequestCache(path)
+	const repoCount = 20
+	var waitGroup sync.WaitGroup
+	errs := make(chan error, repoCount)
+
+	for index := range repoCount {
+		waitGroup.Add(1)
+		go func() {
+			defer waitGroup.Done()
+			repoPath := fmt.Sprintf("/repo/%d", index)
+			errs <- cache.save(repoPath, []CachedPullRequest{{Number: index}})
+		}()
+	}
+	waitGroup.Wait()
+	close(errs)
+	for err := range errs {
+		assert.NoError(t, err)
+	}
+
+	reloadedCache := newGithubPullRequestCache(path)
+	reloadedCache.load()
+	for index := range repoCount {
+		repoPath := fmt.Sprintf("/repo/%d", index)
+		assert.Equal(t, []CachedPullRequest{{Number: index}}, reloadedCache.get(repoPath))
+	}
+	assert.NoError(t, reloadedCache.takeLoadError())
+}

pkg/gui/controllers/helpers/refresh_helper.go

@@ -1776,10 +1776,7 @@ func (self *RefreshHelper) savePullRequestsToCache(prs []*models.GithubPullReque
 		}
 	})
 
-	appState := self.c.GetAppState()
-	if appState.GithubPullRequests == nil {
-		appState.GithubPullRequests = make(map[string][]config.CachedPullRequest)
+	if err := self.c.GetConfig().SaveCachedGithubPullRequests(repoPath, cached); err != nil {
+		self.c.Log.Warnf("error saving GitHub pull request cache: %v", err)
 	}
-	appState.GithubPullRequests[repoPath] = cached
-	self.c.SaveAppStateAndLogError()
 }

pkg/gui/gui.go

@@ -666,7 +666,10 @@ func (gui *Gui) resetState(startArgs appTypes.StartArgs) types.Context {
 
 func (gui *Gui) loadCachedPullRequests() []*models.GithubPullRequest {
 	repoPath := gui.git.RepoPaths.RepoPath()
-	cachedPRs := gui.c.GetAppState().GithubPullRequests[repoPath]
+	cachedPRs, err := gui.Config.GetCachedGithubPullRequests(repoPath)
+	if err != nil {
+		gui.Log.Warnf("error loading GitHub pull request cache: %v", err)
+	}
 
 	return lo.Map(cachedPRs, func(cached config.CachedPullRequest, _ int) *models.GithubPullRequest {
 		return &models.GithubPullRequest{