Skip to content

Commit a9fa574

Browse files
committed
Handle fully merged stacks gracefully in the view and modify TUIs
Merged branches (and their PRs) are not selectable, so once an entire stack has landed there is nothing to act on -- yet the TUIs did not reflect that: - `gh stack view` still drew a highlighted cursor on the top branch even though it could not be selected. Navigation, checkout, and the per-branch toggles all silently did nothing, with no indication of why. - `gh stack modify` opened its full editor on a stack with nothing left to restructure, instead of short-circuiting like `gh stack submit` does when there is nothing to submit. Reflect the "nothing actionable" state in both TUIs. View (internal/tui/stackview/model.go): - Hide the cursor when every branch is merged. `New` now starts the cursor at -1 and only lands it on the current or first non-merged branch; when none exists the cursor stays hidden, so no row is rendered as focused. The existing `m.cursor >= 0` guards and merged-skipping `moveCursor` already make every cursor action a no-op in that state, and mouse-wheel scrolling still works for tall merged stacks. - Dim the shortcuts that depend on the cursor. `buildHeaderConfig` marks navigate, commits, files, open PR, and checkout as `Disabled` (rendered gray via the existing ShortcutEntry.Disabled styling) when all branches are merged, leaving only `q quit` active. Modify (cmd/modify.go): - Short-circuit before opening the TUI. After preconditions pass and PR state is synced, `runModify` now returns early when the stack is fully merged, printing "All branches in this stack have been merged" and pointing at `gh stack init`, exiting cleanly (exit 0) like submit's "nothing to submit" path. The linearity and merge-queue precondition checks already skip merged branches, so they do not fire spuriously. Tests: - internal/tui/stackview/model_test.go: the cursor is hidden (-1) when all branches are merged; up/down/enter do not move it or trigger a checkout; View renders without panicking on a hidden cursor; buildHeaderConfig disables every cursor-dependent shortcut (and only those) when all merged, and leaves them all enabled when active branches remain. - cmd/modify_test.go: runModify short-circuits on a fully merged stack, printing the message and returning no error without launching the TUI.
1 parent 8dd0bdc commit a9fa574

4 files changed

Lines changed: 132 additions & 10 deletions

File tree

cmd/modify.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,14 @@ func runModify(cfg *config.Config) error {
7474
s := result.Stack
7575
currentBranch := result.CurrentBranch
7676

77+
// A fully merged stack has nothing left to restructure. Short-circuit
78+
// before opening the TUI, mirroring submit's "nothing to submit" behavior.
79+
if s.IsFullyMerged() {
80+
cfg.Warningf("All branches in this stack have been merged")
81+
cfg.Printf("There's nothing to modify — start a new stack with `%s`", cfg.ColorCyan("gh stack init"))
82+
return nil
83+
}
84+
7785
// Load branch data for the TUI
7886
viewNodes := stackview.LoadBranchNodes(cfg, s, currentBranch, result.PRDetails)
7987

cmd/modify_test.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package cmd
22

33
import (
44
"encoding/json"
5+
"io"
56
"os"
67
"path/filepath"
78
"testing"
@@ -636,6 +637,50 @@ func TestCheckModifyPreconditions_AllPass(t *testing.T) {
636637
assert.Equal(t, "b1", result.CurrentBranch)
637638
}
638639

640+
func TestRunModify_FullyMergedStack_ShortCircuits(t *testing.T) {
641+
s := stack.Stack{
642+
Trunk: stack.BranchRef{Branch: "main"},
643+
Branches: []stack.BranchRef{
644+
{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 1, Merged: true}},
645+
{Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 2, Merged: true}},
646+
},
647+
}
648+
649+
tmpDir := t.TempDir()
650+
writeStackFile(t, tmpDir, s)
651+
652+
mock := &git.MockOps{
653+
GitDirFn: func() (string, error) { return tmpDir, nil },
654+
CurrentBranchFn: func() (string, error) { return "b1", nil },
655+
IsRebaseInProgressFn: func() bool { return false },
656+
HasUncommittedChangesFn: func() (bool, error) { return false, nil },
657+
BranchExistsFn: func(string) bool { return true },
658+
IsAncestorFn: func(a, d string) (bool, error) { return true, nil },
659+
LogMergesFn: func(base, head string) ([]git.CommitInfo, error) { return nil, nil },
660+
}
661+
restore := git.SetOps(mock)
662+
defer restore()
663+
664+
cfg, _, errR := config.NewTestConfig()
665+
cfg.ForceInteractive = true
666+
cfg.GitHubClientOverride = &github.MockClient{
667+
FindPRForBranchFn: func(string) (*github.PullRequest, error) { return nil, nil },
668+
}
669+
670+
// runModify must short-circuit (and never launch the TUI) on a fully
671+
// merged stack, returning cleanly like submit's "nothing to submit" path.
672+
err := runModify(cfg)
673+
674+
cfg.Out.Close()
675+
cfg.Err.Close()
676+
out, _ := io.ReadAll(errR)
677+
output := string(out)
678+
679+
assert.NoError(t, err)
680+
assert.Contains(t, output, "All branches in this stack have been merged")
681+
assert.Contains(t, output, "gh stack init")
682+
}
683+
639684
// ---------------------------------------------------------------------------
640685
// 5. State file path / exists edge cases
641686
// ---------------------------------------------------------------------------

internal/tui/stackview/model.go

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -83,17 +83,17 @@ func New(nodes []BranchNode, trunk stack.BranchRef, version string) Model {
8383
h := help.New()
8484
h.ShowAll = true
8585

86-
// Cursor starts at the current branch, or first non-merged branch
87-
cursor := 0
88-
found := false
86+
// Cursor starts at the current branch, or the first non-merged branch.
87+
// When every branch is merged there is nothing selectable, so the cursor
88+
// is hidden (-1) and the cursor-dependent shortcuts are disabled.
89+
cursor := -1
8990
for i, n := range nodes {
9091
if n.IsCurrent && !n.Ref.IsMerged() {
9192
cursor = i
92-
found = true
9393
break
9494
}
9595
}
96-
if !found {
96+
if cursor < 0 {
9797
for i, n := range nodes {
9898
if !n.Ref.IsMerged() {
9999
cursor = i
@@ -439,6 +439,10 @@ func (m Model) buildHeaderConfig() shared.HeaderConfig {
439439
branchIcon = "●"
440440
}
441441

442+
// When every branch is merged there is no selectable branch, so the cursor
443+
// is hidden and the actions that depend on it are dimmed; only quit works.
444+
allMerged := branchCount > 0 && mergedCount == branchCount
445+
442446
return shared.HeaderConfig{
443447
ShowArt: true,
444448
Title: "View Stack",
@@ -450,11 +454,11 @@ func (m Model) buildHeaderConfig() shared.HeaderConfig {
450454
},
451455
ShortcutColumns: 1,
452456
Shortcuts: []shared.ShortcutEntry{
453-
{Key: "↑↓", Desc: "navigate"},
454-
{Key: "c", Desc: "commits"},
455-
{Key: "f", Desc: "files"},
456-
{Key: "o", Desc: "open PR"},
457-
{Key: "↵", Desc: "checkout"},
457+
{Key: "↑↓", Desc: "navigate", Disabled: allMerged},
458+
{Key: "c", Desc: "commits", Disabled: allMerged},
459+
{Key: "f", Desc: "files", Disabled: allMerged},
460+
{Key: "o", Desc: "open PR", Disabled: allMerged},
461+
{Key: "↵", Desc: "checkout", Disabled: allMerged},
458462
{Key: "q", Desc: "quit"},
459463
},
460464
}

internal/tui/stackview/model_test.go

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
ghapi "github.com/github/gh-stack/internal/github"
1010
"github.com/github/gh-stack/internal/stack"
1111
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
1213
)
1314

1415
func makeNodes(branches ...string) []BranchNode {
@@ -388,3 +389,67 @@ func TestUpdate_EnterOnMergedDoesNothing(t *testing.T) {
388389
assert.Equal(t, "", m.CheckoutBranch(), "enter on merged branch should not set checkout")
389390
assert.Nil(t, cmd, "enter on merged branch should not quit")
390391
}
392+
393+
// makeAllMergedNodes returns nodes whose branches are all merged.
394+
func makeAllMergedNodes(branches ...string) []BranchNode {
395+
nodes := makeNodes(branches...)
396+
for i := range nodes {
397+
nodes[i].Ref.PullRequest = &stack.PullRequestRef{Number: i + 1, Merged: true}
398+
}
399+
return nodes
400+
}
401+
402+
func TestNew_CursorHiddenWhenAllMerged(t *testing.T) {
403+
m := New(makeAllMergedNodes("b1", "b2"), testTrunk, "0.0.1")
404+
assert.Equal(t, -1, m.cursor, "cursor should be hidden when every branch is merged")
405+
}
406+
407+
func TestUpdate_AllMergedCursorStaysHidden(t *testing.T) {
408+
m := New(makeAllMergedNodes("b1", "b2", "b3"), testTrunk, "0.0.1")
409+
410+
updated, _ := m.Update(keyMsg("down"))
411+
m = updated.(Model)
412+
assert.Equal(t, -1, m.cursor, "down should not move the hidden cursor")
413+
414+
updated, _ = m.Update(keyMsg("up"))
415+
m = updated.(Model)
416+
assert.Equal(t, -1, m.cursor, "up should not move the hidden cursor")
417+
418+
updated, cmd := m.Update(keyMsg("enter"))
419+
m = updated.(Model)
420+
assert.Equal(t, "", m.CheckoutBranch(), "enter should not check out when all merged")
421+
assert.Nil(t, cmd, "enter should not quit when all merged")
422+
}
423+
424+
func TestView_AllMergedRendersWithoutPanic(t *testing.T) {
425+
m := New(makeAllMergedNodes("b1", "b2"), testTrunk, "0.0.1")
426+
updated, _ := m.Update(tea.WindowSizeMsg{Width: 100, Height: 40})
427+
m = updated.(Model)
428+
// Should not panic with a hidden (-1) cursor.
429+
view := m.View()
430+
assert.Contains(t, view, "b1")
431+
}
432+
433+
func TestBuildHeaderConfig_DisablesShortcutsWhenAllMerged(t *testing.T) {
434+
m := New(makeAllMergedNodes("b1", "b2"), testTrunk, "0.0.1")
435+
cfg := m.buildHeaderConfig()
436+
437+
require.NotEmpty(t, cfg.Shortcuts)
438+
for _, sc := range cfg.Shortcuts {
439+
if sc.Desc == "quit" {
440+
assert.False(t, sc.Disabled, "quit should stay enabled")
441+
} else {
442+
assert.True(t, sc.Disabled, "%q should be disabled when all branches merged", sc.Desc)
443+
}
444+
}
445+
}
446+
447+
func TestBuildHeaderConfig_ShortcutsEnabledWithActiveBranches(t *testing.T) {
448+
m := New(makeNodes("b1", "b2"), testTrunk, "0.0.1")
449+
cfg := m.buildHeaderConfig()
450+
451+
require.NotEmpty(t, cfg.Shortcuts)
452+
for _, sc := range cfg.Shortcuts {
453+
assert.False(t, sc.Disabled, "%q should be enabled when there are active branches", sc.Desc)
454+
}
455+
}

0 commit comments

Comments
 (0)