From a141746c94dec828fcf42cf9c4c749982e219d08 Mon Sep 17 00:00:00 2001 From: Nick Borromeo Date: Sat, 31 Oct 2026 18:01:15 -0700 Subject: [PATCH 1/7] Add remoteStacksPRs struct This adds a new struct that will hold information so we can store any data from an API pull and reuse it. --- cmd/utils.go | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/cmd/utils.go b/cmd/utils.go index db43aa9..4f9b9fb 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -767,18 +767,22 @@ func syncStackPRsFromRemote(client github.ClientOps, s *stack.Stack) (map[string return nil, false } - // Find our stack in the remote list. - var remotePRNumbers []int - for _, rs := range stacks { - if strconv.Itoa(rs.ID) == s.ID { - remotePRNumbers = rs.PRNumbers() - // Backfill the human-facing stack number for stack files created - // before it was tracked, so callers (view, submit TUI) can display - // it. Persisted by whichever command later saves the stack file. - if s.Number == 0 { - s.Number = rs.Number - } - break +// remoteStackPRs is one fetch of a tracked stack's pull requests. +// - numbers lists the stack's PRs from bottom to top +// - queried appends merged PRs that are no longer in the stack but still need their head SHA +// - prs is parallel to queried and holds nil where a lookup failed +type remoteStackPRs struct { + numbers []int + queried []int + prs []*github.PullRequest +} + +// stackPRs returns the stack's PRs in order, or false if any lookup failed. +func (r *remoteStackPRs) stackPRs() ([]*github.PullRequest, bool) { + prs := r.prs[:len(r.numbers)] + for _, pr := range prs { + if pr == nil { + return nil, false } } if remotePRNumbers == nil { From 55c36e613afc9278d953af8904b84faa17a222e1 Mon Sep 17 00:00:00 2001 From: Nick Borromeo Date: Sat, 31 Oct 2026 18:06:41 -0700 Subject: [PATCH 2/7] break out stackPRs into smaller chunks --- cmd/utils.go | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/cmd/utils.go b/cmd/utils.go index 4f9b9fb..2647275 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -785,13 +785,29 @@ func (r *remoteStackPRs) stackPRs() ([]*github.PullRequest, bool) { return nil, false } } - if remotePRNumbers == nil { + return prs, true +} + +// fetchRemoteStackPRs reads the tracked stack by number, then fetches its PRs concurrently. +// It returns false when the stack can't be read. +func fetchRemoteStackPRs(client github.ClientOps, s *stack.Stack) (*remoteStackPRs, bool) { + if s.ID == "" { + return nil, false + } + + number, err := ensureStackNumber(client, s) + if err != nil || number == 0 { + return nil, false + } + rs, err := client.GetStack(number) + if err != nil || rs == nil || strconv.Itoa(rs.ID) != s.ID || rs.PRNumbers() == nil { return nil, false } - remotePRCount := len(remotePRNumbers) - queried := make(map[int]bool, remotePRCount) - for _, number := range remotePRNumbers { + numbers := rs.PRNumbers() + remotePRNumbers := slices.Clone(numbers) + queried := make(map[int]bool, len(numbers)) + for _, number := range numbers { queried[number] = true } for _, b := range s.Branches { @@ -802,10 +818,7 @@ func (r *remoteStackPRs) stackPRs() ([]*github.PullRequest, bool) { } } - // Fetch each remote PR concurrently. Results are written to an ordered - // slice (one slot per PR number) so that when we build the branch map - // below, later entries win on duplicate HeadRefNames — matching the - // sequential behavior of the old code. + // Fetch each remote PR concurrently. prResults := make([]*github.PullRequest, len(remotePRNumbers)) var wg sync.WaitGroup From c73b266dcf2998372d2b9607aee2f7143cbdab1f Mon Sep 17 00:00:00 2001 From: Nick Borromeo Date: Sat, 31 Oct 2026 18:08:20 -0700 Subject: [PATCH 3/7] more breaking out of smaller functions --- cmd/utils.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/cmd/utils.go b/cmd/utils.go index 2647275..9bf563c 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -843,6 +843,16 @@ func fetchRemoteStackPRs(client github.ClientOps, s *stack.Stack) (*remoteStackP } wg.Wait() + return &remoteStackPRs{numbers: numbers, queried: remotePRNumbers, prs: prResults}, true +} + +// applyRemoteStackPRs updates the local branches from a remote stack fetch and +// returns PRDetails keyed by branch name. +func applyRemoteStackPRs(s *stack.Stack, fetched *remoteStackPRs) map[string]*github.PRDetails { + remotePRCount := len(fetched.numbers) + prResults := fetched.prs + remotePRNumbers := fetched.queried + // Build map sequentially to preserve order semantics. prByBranch := make(map[string]*github.PullRequest, len(remotePRNumbers)) prByNumber := make(map[int]*github.PullRequest, len(remotePRNumbers)) From 971c12751ee053861123fb14107434421059d6f0 Mon Sep 17 00:00:00 2001 From: Nick Borromeo Date: Sat, 31 Oct 2026 18:09:01 -0700 Subject: [PATCH 4/7] start using the new functions --- cmd/utils.go | 32 ++++++++++---------------------- 1 file changed, 10 insertions(+), 22 deletions(-) diff --git a/cmd/utils.go b/cmd/utils.go index 9bf563c..a922d2c 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -884,7 +884,7 @@ func applyRemoteStackPRs(s *stack.Stack, fetched *remoteStackPRs) map[string]*gi details[b.Branch] = prDetailsFromPR(pr) } - return details, true + return details } // updateBaseSHAs refreshes the Base and Head SHAs for all active branches @@ -2080,6 +2080,10 @@ type remoteReconcileResult struct { // cancels or deletes the remote stack, and when a divergence is detected in a // non-interactive terminal. The resolving path prints its own outcome message. stop bool + + // remotePRs, when non-nil, is the PR fetch reconcile made for a stack it + // left unchanged, so runSync can refresh PR state without fetching again. + remotePRs *remoteStackPRs } // remoteStackClass classifies the relationship between the local stack's active @@ -2113,29 +2117,12 @@ func reconcileRemoteStack(cfg *config.Config, sf *stack.StackFile, s *stack.Stac return res, nil } - stacks, err := client.ListStacks() - if err != nil { - // Covers 404 (stacked PRs unavailable) and transient API errors. - return res, nil - } - - var remotePRNumbers []int - found := false - for _, rs := range stacks { - if strconv.Itoa(rs.ID) == s.ID { - remotePRNumbers = rs.PRNumbers() - found = true - break - } - } - if !found { - // The remote stack was deleted; the existing updateStack 404 → recreate - // path in syncStack handles this. + fetched, ok := fetchRemoteStackPRs(client, s) + if !ok { return res, nil } - - prs, err := fetchStackPRDetails(client, remotePRNumbers) - if err != nil { + prs, ok := fetched.stackPRs() + if !ok { return res, nil } @@ -2148,6 +2135,7 @@ func reconcileRemoteStack(cfg *config.Config, sf *stack.StackFile, s *stack.Stac // fast-forward/rebase/push steps skip merged and merge-queued branches // rather than rewriting them before the later PR-sync step runs. syncRemotePRState(s, prs) + res.remotePRs = fetched return res, nil case remoteStackCleanAhead: return pullRemoteAdditions(cfg, sf, s, gitDir, remote, prs) From 7d54c6b670224313b625607a3e11a18d88b82e82 Mon Sep 17 00:00:00 2001 From: Nick Borromeo Date: Sat, 31 Oct 2026 18:09:16 -0700 Subject: [PATCH 5/7] Replace the ListStacks call with the smaller helper functions --- cmd/utils.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cmd/utils.go b/cmd/utils.go index a922d2c..3d842cb 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -762,10 +762,12 @@ func enrichPRContent(client github.ClientOps, details map[string]*github.PRDetai // closed. Returns the PRDetails map and true if sync succeeded, or nil and // false if we should fall back to local discovery. func syncStackPRsFromRemote(client github.ClientOps, s *stack.Stack) (map[string]*github.PRDetails, bool) { - stacks, err := client.ListStacks() - if err != nil { + fetched, ok := fetchRemoteStackPRs(client, s) + if !ok { return nil, false } + return applyRemoteStackPRs(s, fetched), true +} // remoteStackPRs is one fetch of a tracked stack's pull requests. // - numbers lists the stack's PRs from bottom to top From cae9f9ae5b90a07898f1595d8f391c9bddc15cfd Mon Sep 17 00:00:00 2001 From: Nick Borromeo Date: Sat, 31 Oct 2026 18:10:00 -0700 Subject: [PATCH 6/7] Optimize the runSync function to make less calls The main change that is being proposed here is to lower the number of calls that this function makes via the API. Prior to this change, it was making - 3 full stack listings plus 3 lookups per PR After this change it now just makes - 1 stack fetch plus 1 lookup per PR --- cmd/sync.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/cmd/sync.go b/cmd/sync.go index b5ece46..fb53c95 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -162,7 +162,12 @@ func runSync(cfg *config.Config, opts *syncOptions) error { if cb, cbErr := git.CurrentBranch(); cbErr == nil { currentBranch = cb } - _ = syncStackPRs(cfg, s) + // Reuse reconcile's fetch when it left the stack unchanged. + if reconcileRes.remotePRs != nil { + _ = applyRemoteStackPRs(s, reconcileRes.remotePRs) + } else { + _ = syncStackPRs(cfg, s) + } newBase, err := retargetedBase(cfg, s, remote) if err != nil { cfg.Errorf("failed to read the stack's base on GitHub: %v", err) @@ -335,10 +340,9 @@ func runSync(cfg *config.Config, opts *syncOptions) error { } } - // --- Step 5: Sync PR state --- + // --- Step 5: Report PR state --- cfg.Printf("") - cfg.Printf("Syncing PRs ...") - _ = syncStackPRs(cfg, s) + cfg.Printf("Reporting PR status ...") // Report PR status for each branch for _, b := range s.Branches { @@ -369,12 +373,12 @@ func runSync(cfg *config.Config, opts *syncOptions) error { } // --- Step 5b: Reconcile the remote stack object --- - // syncStackPRs above only refreshes local PR associations; it does not touch - // the stack object on GitHub. When the branches have open PRs, link them into - // a stack so the remote reflects the local stack. This never opens PRs — that - // is still `gh stack submit`'s job. stackSynced records whether the remote - // stack object actually reflects the local stack, which determines the final - // summary message below. + // The earlier PR refresh only updates local PR associations; it does not + // touch the stack object on GitHub. When the branches have open PRs, link + // them into a stack so the remote reflects the local stack. This never opens + // PRs — that is still `gh stack submit`'s job. stackSynced records whether + // the remote stack object actually reflects the local stack, which determines + // the final summary message below. stackSynced := false if client, err := cfg.GitHubClient(); err == nil { stackSynced = syncStack(cfg, client, s) From 983960cb442b053a5ed226334fd0249fe1cdcb1d Mon Sep 17 00:00:00 2001 From: Nick Borromeo Date: Sat, 31 Oct 2026 18:12:20 -0700 Subject: [PATCH 7/7] Add and modify tests --- cmd/submit_test.go | 19 +++++++++++++++---- cmd/sync_test.go | 45 +++++++++++++++++++++++++++++++++++++++++++++ cmd/utils_test.go | 44 +++++++++++++++++++++++++++----------------- 3 files changed, 87 insertions(+), 21 deletions(-) diff --git a/cmd/submit_test.go b/cmd/submit_test.go index 8c2c50f..0107211 100644 --- a/cmd/submit_test.go +++ b/cmd/submit_test.go @@ -459,6 +459,9 @@ func TestSubmit_ForksWhenRemoteStackFullyMerged(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{{ID: 42, PullRequests: []int{1, 2}}}, nil }, + GetStackFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 42, Number: 42, PullRequests: []int{1, 2}}, nil + }, FindPRByNumberFn: func(n int) (*github.PullRequest, error) { switch n { case 1: @@ -1853,10 +1856,9 @@ func TestSubmit_PreflightCheck_SkippedWhenStackIDSet(t *testing.T) { _, _ = io.ReadAll(errR) assert.NoError(t, err) - // ListStacks is called by syncStackPRs (remote sync), but NOT by the - // preflight check. Two syncStackPRs calls happen in submit (before and - // after PR creation), so expect exactly 2 ListStacks calls. - assert.Equal(t, 2, listStacksCallCount, "ListStacks should only be called by syncStackPRs, not by the preflight check") + // With the stack number known, neither the preflight check nor the PR + // refresh needs to scan every stack in the repository. + assert.Equal(t, 0, listStacksCallCount, "ListStacks should not be called for a tracked stack with a number") } // --- Modify + Submit integration tests --- @@ -2012,6 +2014,9 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) { if number == 7 && !oldDeleted { return &github.RemoteStack{ID: 42, Number: 7, PullRequests: oldNumbers}, nil } + if number == 7 && tc.mergedBase { + return &github.RemoteStack{ID: 42, Number: 7, PullRequests: []int{9}}, nil + } if number == 8 && replacement != nil { return replacement, nil } @@ -2661,6 +2666,12 @@ func TestSubmit_WithPendingModify_AtomicPush(t *testing.T) { } return []github.RemoteStack{{ID: 42, Number: 7, PullRequests: []int{10, 11, 12}}}, nil }, + GetStackFn: func(int) (*github.RemoteStack, error) { + if unstacked { + return nil, &api.HTTPError{StatusCode: 404} + } + return &github.RemoteStack{ID: 42, Number: 7, PullRequests: []int{10, 11, 12}}, nil + }, } cmd := SubmitCmd(cfg) diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 063b7e9..e92a6a6 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" "github.com/cli/go-gh/v2/pkg/api" @@ -2214,6 +2215,9 @@ func TestSync_RemoteAhead_DuplicateBranchAborts(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{{ID: 9, Number: 9, PullRequests: []int{101, 102, 103}}}, nil }, + GetStackFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 9, Number: 9, PullRequests: []int{101, 102, 103}}, nil + }, FindPRByNumberFn: prByNumberFinder(map[int]string{101: "b1", 102: "b2", 103: "b3"}), } @@ -2294,6 +2298,47 @@ func TestSync_RemoteInSync_NoPull(t *testing.T) { assert.Contains(t, output, "Stack synced") } +func TestSync_RemoteInSync_ReusesRemotePRFetch(t *testing.T) { + s := stack.Stack{ + ID: "9", + Number: 9, + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}}, + }, + } + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + listStacksCalls := 0 + var mu sync.Mutex + prLookups := map[int]int{} + findPR := prByNumberFinder(map[int]string{101: "b1", 102: "b2"}) + ghMock := &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + listStacksCalls++ + return []github.RemoteStack{{ID: 9, Number: 9, PullRequests: []int{101, 102}}}, nil + }, + GetStackFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 9, Number: 9, PullRequests: []int{101, 102}}, nil + }, + FindPRByNumberFn: func(n int) (*github.PullRequest, error) { + mu.Lock() + prLookups[n]++ + mu.Unlock() + return findPR(n) + }, + } + + output, err := runSyncCfg(t, newSyncMockNoRebase(tmpDir, "b1"), func(cfg *config.Config) { cfg.GitHubClientOverride = ghMock }) + require.NoError(t, err) + assert.Contains(t, output, "Stack synced") + + assert.Zero(t, listStacksCalls, "a numbered stack should not scan every stack in the repository") + assert.Equal(t, map[int]int{101: 1, 102: 1}, prLookups, "each PR should be fetched once per sync") +} + // divergentStack returns a stack file (ID 9) and GitHub mock configured so that // the local stack [b1,b2,b3] diverges from the remote stack [b1,b2,b4]. func divergentStack(t *testing.T, tmpDir string) { diff --git a/cmd/utils_test.go b/cmd/utils_test.go index f421188..555de90 100644 --- a/cmd/utils_test.go +++ b/cmd/utils_test.go @@ -1465,7 +1465,7 @@ func TestSyncStackPRs_RemoteStack_MergedHeadBackfill(t *testing.T) { for _, included := range []bool{false, true} { t.Run(fmt.Sprintf("merged PR listed=%t", included), func(t *testing.T) { s := &stack.Stack{ - ID: "100", Trunk: stack.BranchRef{Branch: "main"}, + ID: "100", Number: 7, Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "parent", PullRequest: &stack.PullRequestRef{Number: 10, Merged: true}}, {Branch: "child", PullRequest: &stack.PullRequestRef{Number: 11}}, @@ -1474,12 +1474,12 @@ func TestSyncStackPRs_RemoteStack_MergedHeadBackfill(t *testing.T) { calls := 0 cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { + GetStackFn: func(int) (*github.RemoteStack, error) { numbers := []int{11} if included { numbers = []int{10, 11} } - return []github.RemoteStack{{ID: 100, PullRequests: numbers}}, nil + return &github.RemoteStack{ID: 100, Number: 7, PullRequests: numbers}, nil }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { if number == 10 { @@ -1625,20 +1625,25 @@ func TestSyncStackPRs_RemoteStack_UsesStackAPI(t *testing.T) { // When the stack has a remote ID, sync should use the stack API // as source of truth, matching PRs to branches by head ref name. s := &stack.Stack{ - ID: "100", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "100", + Number: 7, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "b1"}, {Branch: "b2"}, }, } + var gotNumber int cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{ - {ID: 100, PullRequests: []int{10, 11}}, - }, nil + t.Error("ListStacks should not be called when the stack number is known") + return nil, nil + }, + GetStackFn: func(number int) (*github.RemoteStack, error) { + gotNumber = number + return &github.RemoteStack{ID: 100, Number: 7, PullRequests: []int{10, 11}}, nil }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { switch number { @@ -1654,6 +1659,8 @@ func TestSyncStackPRs_RemoteStack_UsesStackAPI(t *testing.T) { _ = syncStackPRs(cfg, s) collectOutput(cfg, outR, errR) + assert.Equal(t, 7, gotNumber) + // b1 should be tracked with open PR require.NotNil(t, s.Branches[0].PullRequest) assert.Equal(t, 10, s.Branches[0].PullRequest.Number) @@ -1685,6 +1692,9 @@ func TestSyncStackPRs_BackfillsStackNumber(t *testing.T) { {ID: 100, Number: 5, PullRequests: []int{10, 11}}, }, nil }, + GetStackFn: func(number int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 100, Number: number, PullRequests: []int{10, 11}}, nil + }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { switch number { case 10: @@ -1706,8 +1716,9 @@ func TestSyncStackPRs_RemoteStack_ClosedPRStaysAssociated(t *testing.T) { // When using the stack API, a closed (not merged) PR should remain // associated — the stack API is the source of truth, not PR state. s := &stack.Stack{ - ID: "200", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "200", + Number: 2, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "feature", PullRequest: &stack.PullRequestRef{Number: 5}}, }, @@ -1715,10 +1726,8 @@ func TestSyncStackPRs_RemoteStack_ClosedPRStaysAssociated(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{ - {ID: 200, PullRequests: []int{5}}, - }, nil + GetStackFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 200, Number: 2, PullRequests: []int{5}}, nil }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { return &github.PullRequest{Number: 5, ID: "PR_5", URL: "https://github.com/o/r/pull/5", HeadRefName: "feature", State: "CLOSED"}, nil @@ -1737,8 +1746,9 @@ func TestSyncStackPRs_RemoteStack_ClosedPRStaysAssociated(t *testing.T) { func TestSyncStackPRs_RemoteStack_FallsBackOnAPIError(t *testing.T) { // If the stack API fails, fall back to local discovery. s := &stack.Stack{ - ID: "300", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "300", + Number: 3, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "feature"}, }, @@ -1746,7 +1756,7 @@ func TestSyncStackPRs_RemoteStack_FallsBackOnAPIError(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { + GetStackFn: func(int) (*github.RemoteStack, error) { return nil, fmt.Errorf("API error") }, FindPRForBranchFn: func(branch string) (*github.PullRequest, error) {