From 0e7e14e94c92b4996d07310960e9c7f6916adc87 Mon Sep 17 00:00:00 2001 From: Yuval Hayke Date: Tue, 25 Aug 2026 14:46:04 +0300 Subject: [PATCH 1/2] feat(ui): autocomplete the worktree dialog's base branch field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #277. The `w` dialog's New branch field learned to suggest Linear tickets; the Base branch field was still a bare input pre-filled with GetDefaultBranch, so basing a worktree on anything else meant typing the name from memory, exactly. Suggestions now sit under it, on the ticket rows' vocabulary and through the same setSelection highlight. The branch list is fetched ONCE, by fetchWorkspaceListForRepo on the worker goroutine beside the GetDefaultBranch it already ran, so filtering is a synchronous strings.Contains with none of the ticket machinery's debounce or generation guard — those exist only because a lookup is a network round trip. A failed listing leaves the field exactly as it was. Three things the implementation turns on: tab now moves between FIELDS and ↓/↑ walk rows; they used to be one merged case. That was fine while only one field grew rows and stopped being fine once both did — tabbing off a focused base field would have taken six presses to reach the next input. The ↓/↑ walk must retrace itself exactly, which is why ↑ from the New branch field lands on the LAST base row rather than the base input. A suggestion is stored as the exact ref that will be written into the field, so a remote-only branch always comes out origin/-prefixed. `git worktree add -b ` resolves as a plain revision with no remote-tracking DWIM, so a bare remote-only name is a row that looks valid and fails on Enter — and the no-`-b` retry silently drops the base entirely. A field already reading origin/… keeps its prefix and matches only branches with a remote: GetDefaultBranch pre-fills origin/ precisely so a worktree starts from the remote tip, and swapping that for the local branch would change what gets built without saying so. An at-rest field lists the alternatives rather than itself. Filtering the pre-filled origin/ yields exactly one row echoing the field, which answers nothing and no-ops on Enter, so the whole feature was useless on the one screen it matters on. Narrowly gated on exact equality, so a partially typed unique match still filters normally. git.BranchInfo gains HasRemote, which forced ListBranches into two passes: a branch shares its committer date with its origin/ counterpart only when it is level, so a branch you have not pulled has its remote ref sort FIRST — which the single-pass form both missed the flag on and emitted twice. That duplicate was already visible in the `b` key's branch picker. setSelection now guards baseCursor as well, and its AST test scans all three picker files rather than one. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QRVcNGs7c98p7JggANsKNg --- CLAUDE.md | 6 +- .../unreleased/base-branch-autocomplete.md | 5 + internal/git/git.go | 56 ++-- internal/git/git_test.go | 102 ++++++ internal/ui/app.go | 12 +- internal/ui/workspace_picker.go | 129 +++++++- internal/ui/workspace_picker_branch.go | 151 +++++++++ internal/ui/workspace_picker_branch_test.go | 297 ++++++++++++++++++ internal/ui/workspace_picker_layout_test.go | 2 +- internal/ui/workspace_picker_ticket_test.go | 84 +++-- internal/ui/worktree_ticket_e2e_test.go | 6 +- 11 files changed, 775 insertions(+), 75 deletions(-) create mode 100644 changelog/unreleased/base-branch-autocomplete.md create mode 100644 internal/ui/workspace_picker_branch.go create mode 100644 internal/ui/workspace_picker_branch_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 84116064..0d6c4dae 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -160,7 +160,11 @@ Styles in `styles.go` are declared **bare** and constructed **only** in `ApplyPa - **Nothing polls, and a prompt is seeded exactly once.** Ticket work is event-driven and one-shot: it happens at **worktree creation and nowhere else** (`w` + a ticket, `fleet worktree --ticket`), so a session added by hand with `a`/`n`/`A` starts empty like every other manually created session. `handleSessionCreate` therefore does no inference at all — it passes `msg.prompt` through untouched. fleet used to infer a ticket from the branch on *every* session creation, plus reuse the already-written `prompt.txt` when `.fleet/ticket//` was present. Both re-asked the original task on every session after the first, and the reuse branch never looked at the branch at all, so a worktree that once held a ticket kept seeding it after the checkout had moved on to `master`. A seeded first message is the *gesture* of starting a worktree from a ticket, not a property of the directory. `TestTicketWorkStaysOffTheWorkers` keeps it out of `refreshAllGitAndPR`, whose `workerStallThreshold` (90s) is already sized against ~70s of git + `gh` per repo. This is only affordable because there is no badge, hence no live state to keep fresh. - The one mutation resolves the team's started state by **type**, against a position-sorted list (`TestStartedStateResolvesByTypeAndPosition`), so it works on a team whose started state is called "In Dev" or "Doing". Position matters as much as type: a real team has several started states (In Progress at position 2, In Review at 1002) and the lowest is what a human means by "I'm starting this" — any other choice would move a fresh ticket straight to review. Fires **only on create-from-ticket** (config `linear_ticket_start`, default true), never when a later session opens in an existing worktree — by then a human may have moved the issue on, and dragging it backwards is the worst thing this could do. `meta.json` records `state_write` so it stays exactly-once. - The seeded prompt is a **short pointer that tells the agent not to start** (`TestSeedPromptTellsAgentNotToStart`), stated at the top and bottom because a first message describing a task reads as an instruction to perform it. Line 1 leads with the identifier before the title because it is three surfaces at once: the agent's instruction, the preview pane's prompt strip, and the input to `naming.GenerateTitle`, which cuts at ~50 runes. It rides `sessionCreateMsg.prompt` -> `Session.InitialPrompt`. -- **Ticket suggestions live in the `w` dialog's existing New branch field, not a new field and not a mode** (`internal/ui/workspace_picker_ticket.go`). The field IS the literal option, so nothing duplicates it and only one thing ever claims Enter. Two rules make that hold: **exactly one highlight, and the caret lives with it** — arrowing onto a ticket blurs the input, typing returns both and keeps the keystroke (`isTypingKey`, borrowed from the snooze dialog, whose "the highlight is the promise" rule this follows); and **shape decides the default, never a mode** — text matching a team's identifier shape resolves *in place* (`LooksLikeIdentifier`), prose stays literal with tickets one down-arrow below. *In place* means the field **becomes the branch name** (`applyResolvedBranchName` → `BranchNameFor`) while the highlight stays put; it does **not** mean "only record the ticket". That half was missing at first, so typing `BRZ-3217` fetched the ticket, materialized it and named the session after it — and then created a git worktree literally called `BRZ-3217`, breaking the invariant `pickTicket`'s own comment states, that both ways of naming a ticket end up identical. The rewrite is gated on the field still holding **nothing but that identifier**: the generation counter drops a reply a later keystroke invalidated, but it cannot see a *current* reply for a shorter identifier you paused on en route (`BRZ-321` while typing `BRZ-3217`), which would otherwise drop `brz-321-` under the cursor and let the rest of the typing land on the end of it. The same check leaves a tail you edited by hand alone. The confirmation line reads **`✓ named from BRZ-3217 · `**, not a bare identifier: it renders in the same place the selectable ticket rows do, so on its own it read as a row you might still need to arrow onto when the naming had already happened — and it is the only thing on screen that explains why the field rewrote itself a moment earlier. The wording holds in both states `d.resolved` can be in, including a tail you typed yourself, because `onFieldChanged` drops the resolution the moment the text stops leading with the identifier. `✓` is U+2713, East-Asian-Neutral, so it is always one column — the same width check the priority gauge had to pass. The highlight never moves on its own; a picker that jumps its own selection is the ambiguity coming back through the window. `setSelection` is the single writer of `focus`/`ticketCursor`, enforced by `TestWorktreeSelectionMutatorIsTheOnlyWriter`, because a stray write skips the clamp and renders two selection markers. The footer names what Enter will do and changes as the highlight moves. +- **Base branch autocomplete** (`internal/ui/workspace_picker_branch.go`, issue #277): the `w` dialog's Base branch field suggests branches beneath itself, on the ticket rows' vocabulary (`▸` + `selTitle`, dim otherwise, `branchMaxRows`=5) and through the same `setSelection` highlight. The list is fetched **once**, by `fetchWorkspaceListForRepo` on the worker goroutine beside the `GetDefaultBranch` it already ran, and rides `workspaceListMsg.branches` — so filtering is a synchronous `strings.Contains`, with **none** of the ticket machinery's debounce or generation guard, which exist only because a lookup is a network round trip. A failed listing leaves the field exactly as it was. Rows render **only while that field carries the highlight**: the field always holds text, so rendering them unconditionally would park five rows mid-dialog for every user including everyone who never touches the base branch — deliberately unlike `renderTicketBlock`, which renders whenever tickets exist because prose in the *other* field is what put them there. +- **`tab` moves between fields, `↓`/`↑` walk rows** — split apart for this. They were one merged case, which was fine while only the New branch field grew rows and stopped being fine the moment the Base branch field grew its own: tabbing off a focused base field would have taken six presses to reach the next input. The `↓`/`↑` walk is the full continuous path (base input → base rows → new-branch input → ticket rows → worktree list) and must retrace itself exactly — which is why `↑` from the New branch field lands on the *last* base row (`visibleBranchCount()-1`), not the base input. Cost: `tab` no longer walks the worktree list; `↓` still does. `visibleBranchCount()` is deliberately **not** focus-gated even though `renderBranchBlock` is — that `↑` handler asks for the last row while focus is still elsewhere, and a focus gate would answer 0 and silently land on the input. +- **A suggestion is stored as the exact ref that will be written into the field**, which is why a remote-only branch always comes out `origin/`-prefixed (`baseRefFor`). `GitWorktreeProvider.Create` runs `git worktree add <path> -b <new> <base>`, and with `-b` present git resolves `<base>` as a plain revision with **no** remote-tracking DWIM — so a bare remote-only name is a row that looks valid and fails on Enter, and the no-`-b` retry silently drops the base entirely. A field already reading `origin/…` keeps its prefix and matches only branches with a remote (`git.BranchInfo.HasRemote`, added for this): `GetDefaultBranch` pre-fills `origin/<default>` precisely so a worktree starts from the remote tip, and quietly swapping that for the local branch would change what gets built without saying so. `HasRemote` forced `ListBranches` into two passes — a branch shares its committer date with its `origin/` counterpart only when it is *level*, so a branch you have not pulled has its remote ref sort **first**, which the old single-pass form both missed the flag on and emitted twice (`TestListBranchesRemoteCounterparts`; the duplicate was already visible in the `b` key's picker). +- **An at-rest field lists the alternatives, not itself.** Filtering the pre-filled `origin/<default>` yields exactly one row echoing the field — a row that answers nothing and no-ops on Enter — so when the whole match set is that single echo, `rebuildBranchMatches` widens to the unfiltered list (keeping `wantRemote`: you asked for a remote ref, so the wider list is the other remote refs). Narrowly gated on **exact** equality with the field, so a partially-typed unique match still filters normally and the list never widens under your cursor mid-word. +- **Ticket suggestions live in the `w` dialog's existing New branch field, not a new field and not a mode** (`internal/ui/workspace_picker_ticket.go`). The field IS the literal option, so nothing duplicates it and only one thing ever claims Enter. Two rules make that hold: **exactly one highlight, and the caret lives with it** — arrowing onto a ticket blurs the input, typing returns both and keeps the keystroke (`isTypingKey`, borrowed from the snooze dialog, whose "the highlight is the promise" rule this follows); and **shape decides the default, never a mode** — text matching a team's identifier shape resolves *in place* (`LooksLikeIdentifier`), prose stays literal with tickets one down-arrow below. *In place* means the field **becomes the branch name** (`applyResolvedBranchName` → `BranchNameFor`) while the highlight stays put; it does **not** mean "only record the ticket". That half was missing at first, so typing `BRZ-3217` fetched the ticket, materialized it and named the session after it — and then created a git worktree literally called `BRZ-3217`, breaking the invariant `pickTicket`'s own comment states, that both ways of naming a ticket end up identical. The rewrite is gated on the field still holding **nothing but that identifier**: the generation counter drops a reply a later keystroke invalidated, but it cannot see a *current* reply for a shorter identifier you paused on en route (`BRZ-321` while typing `BRZ-3217`), which would otherwise drop `brz-321-<slug>` under the cursor and let the rest of the typing land on the end of it. The same check leaves a tail you edited by hand alone. The confirmation line reads **`✓ named from BRZ-3217 · <title>`**, not a bare identifier: it renders in the same place the selectable ticket rows do, so on its own it read as a row you might still need to arrow onto when the naming had already happened — and it is the only thing on screen that explains why the field rewrote itself a moment earlier. The wording holds in both states `d.resolved` can be in, including a tail you typed yourself, because `onFieldChanged` drops the resolution the moment the text stops leading with the identifier. `✓` is U+2713, East-Asian-Neutral, so it is always one column — the same width check the priority gauge had to pass. The highlight never moves on its own; a picker that jumps its own selection is the ambiguity coming back through the window. `setSelection` is the single writer of `focus`/`ticketCursor`/`baseCursor`, enforced by `TestWorktreeSelectionMutatorIsTheOnlyWriter` across all three picker files, because a stray write skips the clamp and renders two selection markers. The footer names what Enter will do and changes as the highlight moves. - Lookups are **debounced (250ms) and generation-guarded**, never per keystroke. Without the generation counter, typing `BRZ-3182` then editing to `BRZ-3184` lets the slower first reply overwrite the field with the wrong ticket's branch name, which then becomes a real git branch (`TestWorktreeStaleTicketReplyIgnored`). The counter is **monotonic and never reset to 0**, unlike `ConfirmDialog.scanGen` — that one lives on `Home` and never recycles; a per-dialog counter reset to zero would let a reply from a previous open match a new one. `d.loading` is deliberately not used for ticket lookups: it swallows every key but `esc`. - Degradation is structural: no credential, no team, rejected key, offline, unknown id, or nothing matching — every case leaves Enter working and the dialog usable (`TestWorktreeEnterAlwaysCreates`). `ErrNotConnected` is the resting state for everyone who never connected and is **never surfaced as an error**; `ErrNotAuthenticated` is, because it names a thing to fix. Persistent failures latch (`ticketsOff`) so a broken credential isn't re-spent on every pause. Once the worktree exists nothing may fail the caller: `Materialize` sits beside `copyClaudeSettingsFile` and `CopyConfiguredFiles` and shares their posture, and a failed fetch costs the prompt, never the session. - Connect dialog (`internal/ui/connect_linear.go`): a two-row method chooser, then a browser round trip or a **masked** input. A pasted key is **verified before it is stored** (one `viewer`/`organization`/`teams` query), so "connected" is a fact rather than a hope and a typo is caught while the user is still looking at the field they typed it into. It then names the workspace's team keys and shows the exact `.fleet.local.json` line that turns a repo on — the per-repo step is the one people miss, and `.local` is the right file because team membership is personal and `.fleet.json` is committed. Discovery is a `tipOnce` firing when a session's branch matches the generic `<team>-<number>` shape **and** nothing is connected — deliberately *not* `IdentifierFromBranch`, which is gated on team keys nobody has configured yet at the moment the tip should fire; a false positive costs one dismissible hint, since the tip only offers and never fetches. diff --git a/changelog/unreleased/base-branch-autocomplete.md b/changelog/unreleased/base-branch-autocomplete.md new file mode 100644 index 00000000..ad285e2c --- /dev/null +++ b/changelog/unreleased/base-branch-autocomplete.md @@ -0,0 +1,5 @@ +--- +type: improved +--- + +**Base branch autocomplete** — the `w` dialog's Base branch field now suggests your branches. `↓` walks them, `⏎` picks one, `tab` moves to the next field. diff --git a/internal/git/git.go b/internal/git/git.go index 64419112..0354bb71 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -57,6 +57,7 @@ type BranchInfo struct { Name string IsRemote bool // only exists as remote (no local) IsCurrent bool + HasRemote bool // an origin/<Name> ref exists (implied by IsRemote) CommitDate time.Time AuthorEmail string // email of the last commit's author } @@ -75,8 +76,20 @@ func ListBranches(repoPath string) ([]BranchInfo, error) { currentBranch := GetBranchName(repoPath) + // Parsed in one pass, emitted in a second. The refs arrive sorted by commit + // date, and a branch shares its date with its origin/ counterpart, so the two + // can arrive in either order. Deciding inline could only ever see a + // counterpart that happened to sort first — which both missed HasRemote and + // emitted the branch twice. + type parsedRef struct { + name string + remote bool + commitDate time.Time + authorEmail string + } + var refs []parsedRef localSet := make(map[string]bool) - var branches []BranchInfo + remoteSet := make(map[string]bool) lines := strings.Split(strings.TrimSpace(string(output)), "\n") for _, line := range lines { @@ -96,31 +109,32 @@ func ListBranches(repoPath string) ([]BranchInfo, error) { authorEmail = strings.Trim(parts[2], "<>") } - if strings.HasPrefix(name, "origin/") { - remoteName := strings.TrimPrefix(name, "origin/") - if remoteName == "HEAD" { + remote := strings.HasPrefix(name, "origin/") + if remote { + name = strings.TrimPrefix(name, "origin/") + if name == "HEAD" { continue } - if localSet[remoteName] { - continue // already have local version - } - branches = append(branches, BranchInfo{ - Name: remoteName, - IsRemote: true, - IsCurrent: remoteName == currentBranch, - CommitDate: commitDate, - AuthorEmail: authorEmail, - }) + remoteSet[name] = true } else { localSet[name] = true - branches = append(branches, BranchInfo{ - Name: name, - IsRemote: false, - IsCurrent: name == currentBranch, - CommitDate: commitDate, - AuthorEmail: authorEmail, - }) } + refs = append(refs, parsedRef{name: name, remote: remote, commitDate: commitDate, authorEmail: authorEmail}) + } + + var branches []BranchInfo + for _, r := range refs { + if r.remote && localSet[r.name] { + continue // already have local version + } + branches = append(branches, BranchInfo{ + Name: r.name, + IsRemote: r.remote, + IsCurrent: r.name == currentBranch, + HasRemote: remoteSet[r.name], + CommitDate: r.commitDate, + AuthorEmail: r.authorEmail, + }) } // Move current branch to index 0. diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 06bbad11..5a5989d6 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -1,8 +1,10 @@ package git import ( + "os" "os/exec" "path/filepath" + "strings" "testing" ) @@ -74,3 +76,103 @@ func TestGetMainWorktreePathNonRepo(t *testing.T) { t.Errorf("GetMainWorktreePath(non-repo) = %q, want %q", got, dir) } } + +// initBranchRepo builds a repo whose branches cover every shape ListBranches +// has to tell apart: local-only, local with a remote counterpart that is level, +// local with a remote counterpart that is AHEAD, and remote-only. +func initBranchRepo(t *testing.T) string { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + repo := t.TempDir() + // The commit dates are pinned rather than left to the clock. Two + // --allow-empty commits land in the same second, and --sort=-committerdate + // then falls back to refname order, which always puts refs/heads before + // refs/remotes — silently hiding the very ordering this fixture exists to + // produce. + at := func(date string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = repo + if date != "" { + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_DATE="+date, + "GIT_COMMITTER_DATE="+date) + } + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v failed: %v\n%s", args, err, out) + } + return strings.TrimSpace(string(out)) + } + run := func(args ...string) string { return at("", args...) } + + const old, recent = "2020-01-01T00:00:00Z", "2024-01-01T00:00:00Z" + + run("init", "-q", ".") + run("config", "user.email", "test@example.com") + run("config", "user.name", "Test") + at(old, "commit", "-q", "--allow-empty", "-m", "one") + first := run("rev-parse", "HEAD") + + run("branch", "level") + run("branch", "lagging") + run("branch", "solo") + run("update-ref", "refs/remotes/origin/level", first) + + // A newer commit, so origin/lagging sorts strictly BEFORE the local branch + // of the same name — the everyday state of a branch you have not pulled, + // and the ordering that used to defeat the dedupe. + at(recent, "commit", "-q", "--allow-empty", "-m", "two") + second := run("rev-parse", "HEAD") + run("update-ref", "refs/remotes/origin/lagging", second) + run("update-ref", "refs/remotes/origin/ghost", second) + + return repo +} + +// TestListBranchesRemoteCounterparts pins both halves of the two-pass form: the +// HasRemote flag the worktree dialog's origin/ rule reads, and the dedupe — a +// remote ref that sorts BEFORE its local counterpart (because the local branch +// has fallen behind) used to emit the branch twice. +func TestListBranchesRemoteCounterparts(t *testing.T) { + repo := initBranchRepo(t) + + branches, err := ListBranches(repo) + if err != nil { + t.Fatalf("ListBranches: %v", err) + } + + byName := make(map[string][]BranchInfo) + for _, b := range branches { + byName[b.Name] = append(byName[b.Name], b) + } + + cases := []struct { + name string + isRemote bool + hasRemote bool + why string + }{ + {"solo", false, false, "local branch with no origin/ ref"}, + {"level", false, true, "local branch whose origin/ ref is level with it"}, + {"lagging", false, true, "local branch whose origin/ ref is AHEAD, so the remote line sorts first"}, + {"ghost", true, true, "origin/-only branch"}, + } + + for _, c := range cases { + got := byName[c.name] + if len(got) != 1 { + t.Errorf("%s (%s): listed %d times, want exactly 1", c.name, c.why, len(got)) + continue + } + if got[0].IsRemote != c.isRemote { + t.Errorf("%s: IsRemote = %v, want %v (%s)", c.name, got[0].IsRemote, c.isRemote, c.why) + } + if got[0].HasRemote != c.hasRemote { + t.Errorf("%s: HasRemote = %v, want %v (%s)", c.name, got[0].HasRemote, c.hasRemote, c.why) + } + } +} diff --git a/internal/ui/app.go b/internal/ui/app.go index a2f9c110..75e4d072 100644 --- a/internal/ui/app.go +++ b/internal/ui/app.go @@ -1554,7 +1554,7 @@ func (h *Home) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return true }) } - h.worktreeDialog.Show(msg.workspaces, h.sessions, msg.provider, msg.repoPath, msg.defaultBranch, msg.linearTeams) + h.worktreeDialog.Show(msg.workspaces, h.sessions, msg.provider, msg.repoPath, msg.defaultBranch, msg.linearTeams, msg.branches) if id := h.pendingTicketID; id != "" { // Consumed here and nowhere else, so a ticket picked once cannot // leak into the next unrelated `w`. @@ -7205,9 +7205,17 @@ func (h *Home) fetchWorkspaceListForRepo(repoPath string) tea.Cmd { if linear.Available() { linearTeams = linear.TeamKeys(repoPath) } + // Same goroutine, same reason — and unlike the worktree list this one is + // allowed to fail quietly: no branches simply means the Base branch field + // has no suggestions, never a dialog that refuses to open. + branches, branchErr := git.ListBranches(repoPath) + if branchErr != nil { + branches = nil + } return workspaceListMsg{ workspaces: workspaces, provider: provider, repoPath: repoPath, - defaultBranch: defaultBranch, originKey: originKey, linearTeams: linearTeams, err: err, + defaultBranch: defaultBranch, originKey: originKey, linearTeams: linearTeams, + branches: branches, err: err, } } } diff --git a/internal/ui/workspace_picker.go b/internal/ui/workspace_picker.go index efe7d71e..b95d38b8 100644 --- a/internal/ui/workspace_picker.go +++ b/internal/ui/workspace_picker.go @@ -8,6 +8,7 @@ import ( "charm.land/bubbles/v2/textinput" tea "charm.land/bubbletea/v2" "charm.land/lipgloss/v2" + "github.com/brizzai/fleet/internal/git" "github.com/brizzai/fleet/internal/linear" "github.com/brizzai/fleet/internal/session" "github.com/brizzai/fleet/internal/workspace" @@ -26,7 +27,11 @@ type ( // alongside the worktree list. Empty means the repo tracks no Linear // team and every ticket surface below stays inert. linearTeams []string - err error + // branches feeds the Base branch field's suggestions. Resolved off-loop + // too; nil (a failed listing) leaves that field exactly as it was before + // suggestions existed. + branches []git.BranchInfo + err error } workspaceSelectedMsg struct { info workspace.WorkspaceInfo @@ -78,6 +83,24 @@ type WorktreeDialog struct { sessionCounts map[string]int defaultBranch string + // --- Branch suggestions under the Base branch field --- + + // branches is the repo's branch list, fetched once alongside the worktree + // list. Empty makes every surface below inert. + branches []git.BranchInfo + + // branchMatches are the refs the current field text denotes, already in the + // exact form that will be written into the field — see rebuildBranchMatches + // for why a remote-only branch is stored origin/-prefixed. + branchMatches []string + + // baseCursor is the second coordinate of the highlight while focus is + // focusBaseBranch: baseOnInput is the field, 0..n-1 is a row. Mirrors + // ticketCursor, and like it is forced back under any other focus. + baseCursor int + + lastBaseInput string // change detector, so a redraw doesn't refilter + // --- Linear ticket suggestions under the New branch field --- // linearTeams are the team keys this repo tracks. Empty means the whole @@ -128,7 +151,7 @@ func NewWorktreeDialog() *WorktreeDialog { } // Show populates and shows the dialog. -func (d *WorktreeDialog) Show(workspaces []workspace.WorkspaceInfo, sessions []*session.Session, provider workspace.Provider, repoPath, defaultBranch string, linearTeams []string) { +func (d *WorktreeDialog) Show(workspaces []workspace.WorkspaceInfo, sessions []*session.Session, provider workspace.Provider, repoPath, defaultBranch string, linearTeams []string, branches []git.BranchInfo) { d.visible = true d.workspaces = workspaces d.provider = provider @@ -140,6 +163,10 @@ func (d *WorktreeDialog) Show(workspaces []workspace.WorkspaceInfo, sessions []* d.baseBranchInput.SetValue(defaultBranch) d.newBranchInput.SetValue("") + d.branches = branches + d.lastBaseInput = defaultBranch + d.rebuildBranchMatches() + d.linearTeams = linearTeams d.tickets = nil d.resolved = nil @@ -205,7 +232,7 @@ func (d *WorktreeDialog) SetSize(w, h int) { // highlight is the promise". Every navigation key is a one-liner through here, // and TestWorktreeSelectionMutatorIsTheOnlyWriter keeps it that way. // -// Resetting ticketCursor when focus leaves the New-branch region is not +// Resetting ticketCursor and baseCursor when focus leaves their region is not // housekeeping: without it, moving to the worktree list leaves a ▸ on a ticket // row as well as on a worktree row, and nothing downstream catches it. func (d *WorktreeDialog) setSelection(f worktreeFocus, idx int) { @@ -215,8 +242,17 @@ func (d *WorktreeDialog) setSelection(f worktreeFocus, idx int) { } d.focus = f d.ticketCursor = ticketOnInput + d.baseCursor = baseOnInput switch f { + case focusBaseBranch: + if hi := d.visibleBranchCount() - 1; idx > hi { + idx = hi + } + if idx < baseOnInput { + idx = baseOnInput + } + d.baseCursor = idx case focusNewBranch: if hi := d.visibleTicketCount() - 1; idx > hi { idx = hi @@ -232,7 +268,7 @@ func (d *WorktreeDialog) setSelection(f worktreeFocus, idx int) { d.baseBranchInput.Blur() d.newBranchInput.Blur() switch { - case f == focusBaseBranch: + case f == focusBaseBranch && d.baseCursor == baseOnInput: d.baseBranchInput.Focus() case f == focusNewBranch && d.ticketCursor == ticketOnInput: d.newBranchInput.Focus() @@ -272,10 +308,41 @@ func (d *WorktreeDialog) Update(msg tea.Msg) (*WorktreeDialog, tea.Cmd) { d.Hide() return d, nil - case "tab", "down": + // tab moves between the three FIELDS; ↓/↑ walk every row, suggestions + // included. Merging them was fine while only the New branch field grew rows, + // and stopped being fine once the Base branch field grew its own: tabbing off + // a focused base field would have taken six presses to reach the next input. + // Splitting them gives tab one meaning everywhere in this dialog. + case "tab": switch d.focus { case focusBaseBranch: d.setSelection(focusNewBranch, ticketOnInput) + case focusNewBranch: + if len(d.workspaces) > 0 { + d.setSelection(focusWorktreeList, 0) + } + } + return d, nil + + case "shift+tab": + switch d.focus { + case focusNewBranch: + d.setSelection(focusBaseBranch, baseOnInput) + case focusWorktreeList: + d.setSelection(focusNewBranch, ticketOnInput) + } + return d, nil + + case "down": + switch d.focus { + case focusBaseBranch: + // Branch rows sit between the field and the New branch field, so ↓ + // walks into them first when there are any. + if next := d.baseCursor + 1; next < d.visibleBranchCount() { + d.setSelection(focusBaseBranch, next) + } else { + d.setSelection(focusNewBranch, ticketOnInput) + } case focusNewBranch: // Ticket rows sit between the field and the worktree list, so ↓ // walks into them first when there are any. @@ -291,15 +358,20 @@ func (d *WorktreeDialog) Update(msg tea.Msg) (*WorktreeDialog, tea.Cmd) { } return d, nil - case "shift+tab", "up": + case "up": switch d.focus { case focusBaseBranch: - // Already at top, no-op. + if d.baseCursor > baseOnInput { + d.setSelection(focusBaseBranch, d.baseCursor-1) + } + // Already at the top of the dialog otherwise, no-op. case focusNewBranch: if d.ticketCursor > ticketOnInput { d.setSelection(focusNewBranch, d.ticketCursor-1) } else { - d.setSelection(focusBaseBranch, 0) + // The last base row, not the base input — ↓ and ↑ must retrace + // the same path. + d.setSelection(focusBaseBranch, d.visibleBranchCount()-1) } case focusWorktreeList: if d.cursor > 0 { @@ -317,6 +389,13 @@ func (d *WorktreeDialog) Update(msg tea.Msg) (*WorktreeDialog, tea.Cmd) { d.Hide() return d, func() tea.Msg { return workspaceSelectedMsg{info: info} } } + // Enter on a highlighted branch fills the Base field; it does NOT + // create, for the same reason a ticket row doesn't — the other field is + // still empty and the picked ref must stay editable. + if d.focus == focusBaseBranch && d.baseCursor >= 0 && d.baseCursor < len(d.branchMatches) { + d.pickBaseBranch(d.branchMatches[d.baseCursor]) + return d, nil + } // Enter on a highlighted ticket fills the field; it does NOT create. // The base branch may still be wrong and the derived name must stay // editable, so the second Enter is the one that acts — and the footer @@ -351,9 +430,12 @@ func (d *WorktreeDialog) Update(msg tea.Msg) (*WorktreeDialog, tea.Cmd) { } } - // Typing from a ticket row returns the highlight to the field AND keeps the - // keystroke — setSelection runs before the fall-through, so the same message - // is consumed by the input. Same ordering as the snooze dialog. + // Typing from a suggestion row returns the highlight to the field AND keeps + // the keystroke — setSelection runs before the fall-through, so the same + // message is consumed by the input. Same ordering as the snooze dialog. + if d.focus == focusBaseBranch && d.baseCursor != baseOnInput && isTypingKey(keyMsg.String()) { + d.setSelection(focusBaseBranch, baseOnInput) + } if d.focus == focusNewBranch && d.ticketCursor != ticketOnInput && isTypingKey(keyMsg.String()) { d.setSelection(focusNewBranch, ticketOnInput) } @@ -368,6 +450,19 @@ func (d *WorktreeDialog) routeToInput(msg tea.Msg) (*WorktreeDialog, tea.Cmd) { switch d.focus { case focusBaseBranch: d.baseBranchInput, cmd = d.baseBranchInput.Update(msg) + // Change-triggered, not keystroke-triggered, same as the New branch arm + // below. The filter is in-memory, so unlike the ticket lookup it needs + // no debounce and no generation guard. + if current := d.baseBranchInput.Value(); current != d.lastBaseInput { + d.lastBaseInput = current + d.rebuildBranchMatches() + // Only when the list actually shrank out from under the highlight. + // Re-selecting unconditionally would Blur/Focus the input on every + // keystroke and drop the caret-blink command with it. + if d.baseCursor >= d.visibleBranchCount() { + d.setSelection(focusBaseBranch, d.visibleBranchCount()-1) + } + } case focusNewBranch: d.newBranchInput, cmd = d.newBranchInput.Update(msg) current := d.newBranchInput.Value() @@ -412,7 +507,12 @@ func (d *WorktreeDialog) View() string { b.WriteString(DimStyle.Render("Base branch:")) b.WriteString("\n") b.WriteString(d.baseBranchInput.View()) - b.WriteString("\n\n") + b.WriteString("\n") + // Branch suggestions sit directly under the field they fill. Empty unless + // that field carries the highlight, so the dialog opens exactly as it did + // before they existed. + b.WriteString(d.renderBranchBlock(d.innerWidth())) + b.WriteString("\n") // New branch input. The team keys beside the label are the whole // configuration disclosure, a few characters: this repo tracks Linear and @@ -462,7 +562,10 @@ func (d *WorktreeDialog) View() string { // The footer names what Enter does right now, changing as the highlight // moves — so the highlight's promise is also stated in words. Falls back to // the long-standing hint when there is nothing more specific to say. - footer := d.ticketFooter() + footer := d.baseFooter() + if footer == "" { + footer = d.ticketFooter() + } if footer == "" { footer = "tab: next enter: create esc: cancel" } diff --git a/internal/ui/workspace_picker_branch.go b/internal/ui/workspace_picker_branch.go new file mode 100644 index 00000000..5a38ef78 --- /dev/null +++ b/internal/ui/workspace_picker_branch.go @@ -0,0 +1,151 @@ +package ui + +import ( + "strings" + + "github.com/brizzai/fleet/internal/git" + "github.com/charmbracelet/x/ansi" +) + +// baseOnInput is the base-cursor value meaning "the field itself": the caret is +// visible and no row carries the highlight. Mirrors ticketOnInput, kept as its +// own name because it indexes a different set of rows. +const baseOnInput = -1 + +// branchMaxRows caps the suggestion list, matching ticketMaxRows. Small on +// purpose — this completes a field you are already typing in, it is not the +// branch browser the `b` key opens. +const branchMaxRows = 5 + +// remotePrefix is the only remote ListBranches reads, so it is the only one +// suggestions can offer. +const remotePrefix = "origin/" + +// visibleBranchCount is how many rows are rendered while the Base branch field +// carries the highlight, which is what the cursor must be clamped against. +// +// Deliberately NOT gated on focus, unlike renderBranchBlock: the ↑ handler asks +// for the last row while focus is still on the New branch field, and a focus +// gate would answer 0 there and silently land on the input instead. +func (d *WorktreeDialog) visibleBranchCount() int { + return len(d.branchMatches) +} + +// rebuildBranchMatches refilters the branch list against the field's text. +// +// Each match is stored as the exact ref that will be written into the field, +// which is why a remote-only branch comes out origin/-prefixed: the worktree is +// created with `git worktree add <path> -b <new> <base>`, and with -b present +// git resolves <base> as a plain revision with no remote-tracking DWIM. A bare +// remote-only name does not resolve there, so offering one would be a row that +// looks fine and fails on Enter. +// +// A field already reading origin/… keeps its prefix, and only branches that +// have a remote can match it — GetDefaultBranch pre-fills origin/<default> +// precisely so a new worktree starts from the remote tip, and quietly swapping +// that for the local branch would change what gets built without saying so. +func (d *WorktreeDialog) rebuildBranchMatches() { + d.branchMatches = nil + if len(d.branches) == 0 { + return + } + + text := strings.TrimSpace(d.baseBranchInput.Value()) + wantRemote := strings.HasPrefix(text, remotePrefix) + q := strings.ToLower(strings.TrimPrefix(text, remotePrefix)) + + d.branchMatches = d.matchBranches(q, wantRemote) + + // A field sitting on a complete ref is at rest, and the one row echoing it + // back answers a question nobody asked — Enter on it is a no-op. The useful + // question there is "what else is there", so fall back to the unfiltered + // list. This matters because the field is PRE-FILLED with origin/<default>: + // without it, focusing the field for the first time shows a single row + // repeating what is already on the line above it, and browsing would mean + // clearing a field you probably wanted to keep. + if len(d.branchMatches) == 1 && d.branchMatches[0] == text { + // wantRemote is deliberately kept: you asked for a remote ref, so the + // wider list is the other remote refs. + d.branchMatches = d.matchBranches("", wantRemote) + } +} + +// matchBranches returns the refs whose branch name contains q, in the form each +// one takes as a base ref. An empty q matches everything, so an at-rest field +// lists the most recently committed branches — ListBranches is already in that +// order. +func (d *WorktreeDialog) matchBranches(q string, wantRemote bool) []string { + var out []string + for _, b := range d.branches { + if wantRemote && !b.HasRemote { + continue + } + if q != "" && !strings.Contains(strings.ToLower(b.Name), q) { + continue + } + out = append(out, baseRefFor(b, wantRemote)) + if len(out) >= branchMaxRows { + break + } + } + return out +} + +// baseRefFor is the form of a branch that can serve as a base ref. +func baseRefFor(b git.BranchInfo, wantRemote bool) string { + if b.IsRemote || (wantRemote && b.HasRemote) { + return remotePrefix + b.Name + } + return b.Name +} + +// pickBaseBranch fills the field from a highlighted row and returns the +// highlight — and with it the caret — to the field, so the ref stays editable. +// The same shape as pickTicket, for the same reason. +func (d *WorktreeDialog) pickBaseBranch(ref string) { + d.baseBranchInput.SetValue(ref) + d.baseBranchInput.SetCursor(len([]rune(ref))) + // Write the value, then the change detector, or routeToInput refilters + // against text it already knows about. + d.lastBaseInput = ref + d.rebuildBranchMatches() + d.err = "" + d.setSelection(focusBaseBranch, baseOnInput) +} + +// renderBranchBlock renders the suggestion rows under the Base branch field. +// +// Empty unless that field carries the highlight: the field always holds text, so +// rendering unconditionally would park five rows in the middle of the dialog for +// every user, including everyone who never touches the base branch. +func (d *WorktreeDialog) renderBranchBlock(innerW int) string { + if d.focus != focusBaseBranch { + return "" + } + var b strings.Builder + for i, ref := range d.branchMatches { + row := ansi.Truncate(ref, maxInt(innerW-4, 12), "…") + if d.baseCursor == i { + b.WriteString(SelectionMarker(true).Render("▸ ") + selTitle().Render(row)) + } else { + b.WriteString(" " + DimStyle.Render(row)) + } + b.WriteString("\n") + } + return b.String() +} + +// baseFooter names what Enter does while the Base branch field has the +// highlight. Empty means "let the ticket footer, then the default, speak". +func (d *WorktreeDialog) baseFooter() string { + if d.focus != focusBaseBranch { + return "" + } + if d.baseCursor >= 0 && d.baseCursor < len(d.branchMatches) { + return "⏎ use " + d.branchMatches[d.baseCursor] + " ↑ back to typing esc: cancel" + } + if len(d.branchMatches) > 0 { + return "↓ branches tab: next field ⏎ create esc: cancel" + } + return "" +} diff --git a/internal/ui/workspace_picker_branch_test.go b/internal/ui/workspace_picker_branch_test.go new file mode 100644 index 00000000..cd55cb68 --- /dev/null +++ b/internal/ui/workspace_picker_branch_test.go @@ -0,0 +1,297 @@ +package ui + +import ( + "strings" + "testing" + + tea "charm.land/bubbletea/v2" + "github.com/brizzai/fleet/internal/git" + "github.com/brizzai/fleet/internal/workspace" +) + +// branchFixture covers the three shapes that behave differently: local-only, +// local with a remote counterpart, and remote-only. +func branchFixture() []git.BranchInfo { + return []git.BranchInfo{ + {Name: "master", HasRemote: true}, + {Name: "base-branch", HasRemote: true}, + {Name: "fix/local-only"}, + {Name: "feat/remote-only", IsRemote: true, HasRemote: true}, + } +} + +func branchDialog(t *testing.T, branches ...git.BranchInfo) *WorktreeDialog { + t.Helper() + d := NewWorktreeDialog() + d.SetSize(120, 40) + d.Show(nil, nil, nil, "/repo", "origin/master", nil, branches) + return d +} + +var ( + keyDown = tea.KeyPressMsg{Code: tea.KeyDown} + keyUp = tea.KeyPressMsg{Code: tea.KeyUp} + keyTab = tea.KeyPressMsg{Code: tea.KeyTab} + keyShiftTab = tea.KeyPressMsg{Code: tea.KeyTab, Mod: tea.ModShift} + keyEnter = tea.KeyPressMsg{Code: tea.KeyEnter} +) + +func (d *WorktreeDialog) send(msg tea.Msg) *WorktreeDialog { + out, _ := d.Update(msg) + return out +} + +// TestBaseBranchSuggestionsMatchOnName: the ordinary case — type a fragment, +// get the branches containing it, in the form that goes into the field. +func TestBaseBranchSuggestionsMatchOnName(t *testing.T) { + d := branchDialog(t, branchFixture()...) + d.setSelection(focusBaseBranch, baseOnInput) + d.baseBranchInput.SetValue("") + d.lastBaseInput = "" + d.rebuildBranchMatches() + + // Typed rather than injected: this is the path routeToInput's change + // detector actually runs on, and a test that skips it would pass with the + // detector unwired. + for _, r := range "branch" { + d = d.send(tea.KeyPressMsg{Code: r, Text: string(r)}) + } + if got := d.baseBranchInput.Value(); got != "branch" { + t.Fatalf("field = %q after typing, want %q", got, "branch") + } + + want := []string{"base-branch"} + if got := d.branchMatches; !equalStrings(got, want) { + t.Errorf("matches = %v, want %v", got, want) + } +} + +// TestBaseBranchRemoteOnlyIsAlwaysPrefixed is the dead-click guard. `git +// worktree add <path> -b <new> <base>` resolves <base> as a plain revision with +// no remote-tracking DWIM, so a bare remote-only name would fail on Enter after +// looking perfectly valid on screen. +func TestBaseBranchRemoteOnlyIsAlwaysPrefixed(t *testing.T) { + d := branchDialog(t, branchFixture()...) + d.baseBranchInput.SetValue("remote-only") + d.rebuildBranchMatches() + + want := []string{"origin/feat/remote-only"} + if got := d.branchMatches; !equalStrings(got, want) { + t.Errorf("matches = %v, want %v — a remote-only branch must be offered as a ref that resolves", got, want) + } +} + +// TestBaseBranchOriginPrefixIsPreserved: GetDefaultBranch pre-fills +// origin/<default> so a worktree starts from the remote tip. A picker that +// silently swapped that for the local branch would change what gets built. +func TestBaseBranchOriginPrefixIsPreserved(t *testing.T) { + d := branchDialog(t, branchFixture()...) + + d.baseBranchInput.SetValue("origin/") + d.rebuildBranchMatches() + for _, ref := range d.branchMatches { + if !strings.HasPrefix(ref, "origin/") { + t.Errorf("origin/ query offered %q — the prefix the user typed must survive", ref) + } + } + // The local-only branch has no origin/ ref, so it cannot answer this query. + for _, ref := range d.branchMatches { + if strings.Contains(ref, "local-only") { + t.Errorf("origin/ query offered %q, which has no remote", ref) + } + } + + // Without the prefix, the same branch comes back in its local form. Typed + // partially so the at-rest widening (see TestBaseBranchAtRestFieldListsTheRest) + // stays out of the way — this is about the prefix, not the list length. + d.baseBranchInput.SetValue("mast") + d.rebuildBranchMatches() + if got := d.branchMatches; !equalStrings(got, []string{"master"}) { + t.Errorf("bare query = %v, want [master] — no prefix asked for, none added", got) + } +} + +// TestBaseBranchAtRestFieldListsTheRest is why the pre-filled origin/<default> +// does not make this feature useless on the one screen it matters on. Filtering +// that text yields exactly one row echoing the field, which answers nothing and +// does nothing on Enter — so an at-rest field lists the alternatives instead. +func TestBaseBranchAtRestFieldListsTheRest(t *testing.T) { + d := branchDialog(t, branchFixture()...) + + // Show() pre-fills origin/master, which matches origin/master and nothing else. + if len(d.branchMatches) < 2 { + t.Fatalf("field at rest on %q offered %v — want the other branches, not one row repeating the field", + d.baseBranchInput.Value(), d.branchMatches) + } + for _, ref := range d.branchMatches { + if !strings.HasPrefix(ref, "origin/") { + t.Errorf("widened list offered %q — the origin/ the field asked for must survive", ref) + } + } + + // A partial match that is NOT the whole field still filters normally: the + // widening is for the at-rest case only, not a general fallback. + d.baseBranchInput.SetValue("mast") + d.rebuildBranchMatches() + if got := d.branchMatches; !equalStrings(got, []string{"master"}) { + t.Errorf("partial query = %v, want [master] — a unique match must not widen", got) + } +} + +// TestBaseBranchEmptyFieldListsRecent: an emptied field is a browse, not a +// dead end. +func TestBaseBranchEmptyFieldListsRecent(t *testing.T) { + d := branchDialog(t, branchFixture()...) + d.baseBranchInput.SetValue("") + d.rebuildBranchMatches() + if len(d.branchMatches) != len(branchFixture()) { + t.Errorf("empty field matched %d branches, want all %d", len(d.branchMatches), len(branchFixture())) + } +} + +// TestBaseBranchTabSkipsSuggestionRows is the whole point of splitting tab from +// ↓: five suggestion rows must never sit between you and the next field. +func TestBaseBranchTabSkipsSuggestionRows(t *testing.T) { + d := branchDialog(t, branchFixture()...) + d.setSelection(focusBaseBranch, baseOnInput) + if d.visibleBranchCount() == 0 { + t.Fatal("fixture produced no rows, the test proves nothing") + } + + d = d.send(keyTab) + if d.focus != focusNewBranch || d.ticketCursor != ticketOnInput { + t.Errorf("tab landed on focus=%v cursor=%d, want the New branch field", d.focus, d.ticketCursor) + } + + d = d.send(keyShiftTab) + if d.focus != focusBaseBranch || d.baseCursor != baseOnInput { + t.Errorf("shift+tab landed on focus=%v cursor=%d, want the Base branch field", d.focus, d.baseCursor) + } +} + +// TestBaseBranchArrowsWalkRowsBothWays: ↓ and ↑ must retrace the same path, or +// backing out of a suggestion list lands somewhere you were never offered. +func TestBaseBranchArrowsWalkRowsBothWays(t *testing.T) { + d := branchDialog(t, branchFixture()...) + d.setSelection(focusBaseBranch, baseOnInput) + n := d.visibleBranchCount() + + for i := 0; i < n; i++ { + d = d.send(keyDown) + if d.focus != focusBaseBranch || d.baseCursor != i { + t.Fatalf("after %d ↓: focus=%v cursor=%d, want base row %d", i+1, d.focus, d.baseCursor, i) + } + } + d = d.send(keyDown) + if d.focus != focusNewBranch || d.ticketCursor != ticketOnInput { + t.Fatalf("↓ past the last row: focus=%v, want the New branch field", d.focus) + } + + d = d.send(keyUp) + if d.focus != focusBaseBranch || d.baseCursor != n-1 { + t.Fatalf("↑ from the New branch field: focus=%v cursor=%d, want base row %d", d.focus, d.baseCursor, n-1) + } + for i := n - 2; i >= 0; i-- { + d = d.send(keyUp) + if d.baseCursor != i { + t.Fatalf("↑ walk: cursor=%d, want %d", d.baseCursor, i) + } + } + d = d.send(keyUp) + if d.baseCursor != baseOnInput || !d.baseBranchInput.Focused() { + t.Fatalf("↑ from row 0: cursor=%d focused=%v, want the field", d.baseCursor, d.baseBranchInput.Focused()) + } +} + +// TestBaseBranchEnterFillsAndDoesNotCreate: Enter on a row is an accept, not a +// submit — the New branch field is still empty at that point. +func TestBaseBranchEnterFillsAndDoesNotCreate(t *testing.T) { + d := branchDialog(t, branchFixture()...) + d.setSelection(focusBaseBranch, 0) + want := d.branchMatches[0] + + d, cmd := d.Update(keyEnter) + if cmd != nil { + t.Error("enter on a suggestion returned a command — it must not create a worktree") + } + if !d.visible { + t.Error("enter on a suggestion closed the dialog") + } + if got := d.baseBranchInput.Value(); got != want { + t.Errorf("field = %q, want %q", got, want) + } + if d.baseCursor != baseOnInput || !d.baseBranchInput.Focused() { + t.Errorf("after accept: cursor=%d focused=%v, want the highlight and caret back on the field", + d.baseCursor, d.baseBranchInput.Focused()) + } +} + +// TestBaseBranchTypingFromARowKeepsTheKeystroke: the rule the snooze dialog set +// and the ticket rows follow — returning to the field must not eat the letter +// that returned you to it. +func TestBaseBranchTypingFromARowKeepsTheKeystroke(t *testing.T) { + d := branchDialog(t, branchFixture()...) + d.baseBranchInput.SetValue("") + d.rebuildBranchMatches() + d.setSelection(focusBaseBranch, 0) + + d = d.send(key("m")) + if d.baseCursor != baseOnInput { + t.Errorf("typing left the highlight on row %d", d.baseCursor) + } + if got := d.baseBranchInput.Value(); got != "m" { + t.Errorf("field = %q, want %q — the keystroke that moved the highlight must still land", got, "m") + } +} + +// TestBaseBranchRowsHiddenUnlessFocused: the field always holds text, so +// rendering unconditionally would park five rows in the dialog for every user +// including everyone who never touches the base branch. +func TestBaseBranchRowsHiddenUnlessFocused(t *testing.T) { + d := branchDialog(t, branchFixture()...) + d.workspaces = []workspace.WorkspaceInfo{{Name: "wt-a", Path: "/a"}} + + // Show() leaves focus on the New branch field. + if got := d.renderBranchBlock(d.innerWidth()); got != "" { + t.Errorf("rows rendered with focus on the New branch field:\n%s", got) + } + d.setSelection(focusBaseBranch, baseOnInput) + if got := d.renderBranchBlock(d.innerWidth()); got == "" { + t.Error("no rows rendered with the Base branch field focused") + } + if n := strings.Count(d.View(), "▸"); n > 1 { + t.Errorf("%d ▸ markers on screen, want at most 1", n) + } +} + +// TestBaseBranchWithoutBranchesIsInert: a failed `git for-each-ref` must cost +// the suggestions and nothing else. +func TestBaseBranchWithoutBranchesIsInert(t *testing.T) { + with := branchDialog(t) + without := NewWorktreeDialog() + without.SetSize(120, 40) + without.Show(nil, nil, nil, "/repo", "origin/master", nil, nil) + + if with.View() != without.View() { + t.Error("a dialog with no branches renders differently from one that never had any") + } + + d := branchDialog(t) + d.setSelection(focusBaseBranch, baseOnInput) + d = d.send(keyDown) + if d.focus != focusNewBranch { + t.Errorf("↓ with no rows landed on focus=%v, want the New branch field", d.focus) + } +} + +func equalStrings(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/internal/ui/workspace_picker_layout_test.go b/internal/ui/workspace_picker_layout_test.go index abcc6b9b..f0ff992b 100644 --- a/internal/ui/workspace_picker_layout_test.go +++ b/internal/ui/workspace_picker_layout_test.go @@ -111,7 +111,7 @@ func TestWorktreeDialogRowsNeverOverflow(t *testing.T) { for _, w := range []int{40, 60, 80, 100, 120, 200} { d := NewWorktreeDialog() d.SetSize(w, 40) - d.Show(wss, nil, nil, "/Users/y/code/brizzai", "origin/master", nil) + d.Show(wss, nil, nil, "/Users/y/code/brizzai", "origin/master", nil, nil) d.sessionCounts = counts // Ground-truth innerWidth against Lip Gloss itself, rather than against diff --git a/internal/ui/workspace_picker_ticket_test.go b/internal/ui/workspace_picker_ticket_test.go index c7e06a44..e8b59d19 100644 --- a/internal/ui/workspace_picker_ticket_test.go +++ b/internal/ui/workspace_picker_ticket_test.go @@ -17,7 +17,7 @@ func ticketDialog(t *testing.T, tickets ...linear.Ticket) *WorktreeDialog { t.Helper() d := NewWorktreeDialog() d.SetSize(120, 40) - d.Show(nil, nil, nil, "/repo", "master", []string{"BRZ"}) + d.Show(nil, nil, nil, "/repo", "master", []string{"BRZ"}, nil) d.tickets = tickets return d } @@ -32,13 +32,16 @@ func TestWorktreeCaretAndHighlightNeverCoexist(t *testing.T) { linear.Ticket{Identifier: "BRZ-3040", Title: "Collapse resets"}, ) d.workspaces = []workspace.WorkspaceInfo{{Name: "wt-a", Path: "/a"}} + d.branchMatches = []string{"master", "feature/x"} states := []struct { name string f worktreeFocus idx int }{ - {"base branch", focusBaseBranch, 0}, + {"base branch, on input", focusBaseBranch, baseOnInput}, + {"base branch, branch 0", focusBaseBranch, 0}, + {"base branch, branch 1", focusBaseBranch, 1}, {"new branch, on input", focusNewBranch, ticketOnInput}, {"new branch, ticket 0", focusNewBranch, 0}, {"new branch, ticket 1", focusNewBranch, 1}, @@ -53,8 +56,9 @@ func TestWorktreeCaretAndHighlightNeverCoexist(t *testing.T) { t.Errorf("%s: new-branch caret = %v, want %v — the caret must live exactly where the highlight is", s.name, got, onInput) } - if got := d.baseBranchInput.Focused(); got != (d.focus == focusBaseBranch) { - t.Errorf("%s: base caret = %v", s.name, got) + onBase := d.focus == focusBaseBranch && d.baseCursor == baseOnInput + if got := d.baseBranchInput.Focused(); got != onBase { + t.Errorf("%s: base caret = %v, want %v", s.name, got, onBase) } // The render is the thing the user actually reads: exactly one marker. @@ -70,46 +74,58 @@ func TestWorktreeCaretAndHighlightNeverCoexist(t *testing.T) { // and the caret sync, and reintroduces the double highlight. func TestWorktreeSelectionMutatorIsTheOnlyWriter(t *testing.T) { fset := token.NewFileSet() - f, err := parser.ParseFile(fset, "workspace_picker.go", nil, 0) - if err != nil { - t.Fatalf("parse: %v", err) + // Every file that can reach the highlight, not just the one that declares it: + // the suggestion logic lives beside the dialog, and a stray write there is + // exactly as damaging as one here. + var files []*ast.File + for _, name := range []string{"workspace_picker.go", "workspace_picker_branch.go", "workspace_picker_ticket.go"} { + f, err := parser.ParseFile(fset, name, nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", name, err) + } + files = append(files, f) } writesInsideMutator := 0 - ast.Inspect(f, func(n ast.Node) bool { - fn, ok := n.(*ast.FuncDecl) - if !ok || fn.Body == nil { - return true - } - ast.Inspect(fn.Body, func(m ast.Node) bool { - as, ok := m.(*ast.AssignStmt) - if !ok { + inspect := func(f *ast.File) { + ast.Inspect(f, func(n ast.Node) bool { + fn, ok := n.(*ast.FuncDecl) + if !ok || fn.Body == nil { return true } - for _, lhs := range as.Lhs { - sel, ok := lhs.(*ast.SelectorExpr) + ast.Inspect(fn.Body, func(m ast.Node) bool { + as, ok := m.(*ast.AssignStmt) if !ok { - continue + return true } - ident, ok := sel.X.(*ast.Ident) - if !ok || ident.Name != "d" { - continue - } - switch sel.Sel.Name { - case "focus", "ticketCursor": - if fn.Name.Name != "setSelection" { - t.Errorf("%s writes d.%s directly at %s — only setSelection may move the "+ - "highlight, or the caret and the marker drift apart", - fn.Name.Name, sel.Sel.Name, fset.Position(as.Pos())) - } else { - writesInsideMutator++ + for _, lhs := range as.Lhs { + sel, ok := lhs.(*ast.SelectorExpr) + if !ok { + continue + } + ident, ok := sel.X.(*ast.Ident) + if !ok || ident.Name != "d" { + continue + } + switch sel.Sel.Name { + case "focus", "ticketCursor", "baseCursor": + if fn.Name.Name != "setSelection" { + t.Errorf("%s writes d.%s directly at %s — only setSelection may move the "+ + "highlight, or the caret and the marker drift apart", + fn.Name.Name, sel.Sel.Name, fset.Position(as.Pos())) + } else { + writesInsideMutator++ + } } } - } + return true + }) return true }) - return true - }) + } + for _, f := range files { + inspect(f) + } // Positive control: a scanner that finds nothing would pass vacuously. if writesInsideMutator < 2 { @@ -305,7 +321,7 @@ func TestWorktreeBlankRenderUnchangedWithoutLinear(t *testing.T) { mk := func(teams []string) string { d := NewWorktreeDialog() d.SetSize(120, 40) - d.Show(nil, nil, nil, "/repo", "master", teams) + d.Show(nil, nil, nil, "/repo", "master", teams, nil) d.newBranchInput.SetValue("my-experiment") return d.View() } diff --git a/internal/ui/worktree_ticket_e2e_test.go b/internal/ui/worktree_ticket_e2e_test.go index ac529484..fed52e85 100644 --- a/internal/ui/worktree_ticket_e2e_test.go +++ b/internal/ui/worktree_ticket_e2e_test.go @@ -32,7 +32,7 @@ func TestTicketLookupSurvivesTheRealMessageLoop(t *testing.T) { h := NewHome(storage, &config.Config{TickIntervalSec: 2}, "test", analytics.Identity{}) h.width, h.height = 120, 40 h.worktreeDialog.SetSize(120, 40) - h.worktreeDialog.Show(nil, nil, nil, "/repo", "master", []string{"BRZ"}) + h.worktreeDialog.Show(nil, nil, nil, "/repo", "master", []string{"BRZ"}, nil) // Type "sdk" the way a user does: one key at a time, through Home. var pending []tea.Cmd @@ -92,7 +92,7 @@ func TestTicketLookupSurvivesTheRealMessageLoop(t *testing.T) { func TestWrongWorkspaceIsNamedNotSilent(t *testing.T) { d := NewWorktreeDialog() d.SetSize(120, 40) - d.Show(nil, nil, nil, "/repo", "master", []string{"BRZ"}) + d.Show(nil, nil, nil, "/repo", "master", []string{"BRZ"}, nil) // Connected to a workspace that has no BRZ team. linear.SetWorkspaceForTest(linear.Workspace{Name: "fleet", TeamKeys: []string{"FLE"}}) @@ -150,7 +150,7 @@ func TestWorkspaceMismatchNoteFitsOnOneLine(t *testing.T) { t.Run(c.name, func(t *testing.T) { d := NewWorktreeDialog() d.SetSize(120, 40) - d.Show(nil, nil, nil, "/repo", "master", c.teams) + d.Show(nil, nil, nil, "/repo", "master", c.teams, nil) linear.SetWorkspaceForTest(linear.Workspace{Name: c.ws, TeamKeys: []string{"FLE"}}) t.Cleanup(func() { linear.SetWorkspaceForTest(linear.Workspace{}) }) From 38a667b38ac4aab93a0cdc38fa93390f6874d55b Mon Sep 17 00:00:00 2001 From: Yuval Hayke <yuval@brizz.ai> Date: Tue, 25 Aug 2026 16:08:14 +0300 Subject: [PATCH 2/2] fix(ui): address PR #278 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six threads, all verified by probe before fixing; none was a false positive. git.BranchInfo now carries the LATER of the two refs' dates when a branch has an origin/ counterpart. The dedupe keeps the local ref, but a branch you have not pulled has a local tip older than origin/<name> — and baseRefFor hands the remote form to `git worktree add`, so the row described a commit it did not resolve to. It is also the sort key, so an unpulled master sank below fresher topic branches and fell out of the 5-row suggestion list entirely. Branches are re-sorted after the merge, since for-each-ref's order stops being authoritative once a date is revised. This was a regression from the two-pass dedupe: the old duplicate preserved the newer position by accident. The widened at-rest list no longer offers back the value already in the field — that is the same no-op row the widening exists to remove. The exclusion runs before the branchMaxRows cap, not over its result, which would quietly return four rows where five fit. Widening is now gated on the field having SETTLED (set by Show and pickBaseBranch, cleared by the first typed change) rather than on the text equalling a branch name. Those look equivalent and are not: typing toward `master-fix` passes through `master`, so the equality form widened 1 row to 5 at the `r` and collapsed at the `-` — and the dialog is vertically centred, so that was the whole box jumping four rows mid-word. The CLAUDE.md sentence that claimed this could not happen is now true rather than softened. The existing-worktrees list is hidden while the base field has the highlight. The dialog has no height budget, so at 80x24 with six worktrees the suggestion rows took the box from 21 lines to 26 and pushed the footer off the bottom. Dropping the list reclaims more than the rows add, so the focused dialog is strictly shorter than the resting one. Capping rows against remaining height was rejected: setSelection runs nowhere near View, so the cursor clamp would have started varying with window size. Hidden is not unreachable — shift+tab still cycles onto the list. tab/shift+tab now cycle. "tab no longer walks the worktree list" stands as a documented cost, but a key documented as "next field" doing nothing at all on the last field is a dead key, and wrapping one direction only moves the dead end onto the other. ListBranches and GetDefaultBranch move inside the existing !IsCustom() guard. The custom-provider path returns straight to createWorkspaceDialog and reads neither, so both were subprocesses the user waited on behind the spinner on every `w`, against a directory a shell provider need not keep in git. Three tests here poked SetValue + rebuildBranchMatches directly, which left the field at rest and landed their assertions on the wrong branch while claiming to test filtering. They now type through Update like a user, and typeBase asserts the change detector actually ran. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QRVcNGs7c98p7JggANsKNg --- CLAUDE.md | 6 +- internal/git/git.go | 32 ++- internal/git/git_test.go | 42 ++++ internal/ui/app.go | 21 +- internal/ui/workspace_picker.go | 35 ++- internal/ui/workspace_picker_branch.go | 52 +++-- internal/ui/workspace_picker_branch_test.go | 237 ++++++++++++++++++-- 7 files changed, 372 insertions(+), 53 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 0d6c4dae..b9c228a6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -160,10 +160,10 @@ Styles in `styles.go` are declared **bare** and constructed **only** in `ApplyPa - **Nothing polls, and a prompt is seeded exactly once.** Ticket work is event-driven and one-shot: it happens at **worktree creation and nowhere else** (`w` + a ticket, `fleet worktree --ticket`), so a session added by hand with `a`/`n`/`A` starts empty like every other manually created session. `handleSessionCreate` therefore does no inference at all — it passes `msg.prompt` through untouched. fleet used to infer a ticket from the branch on *every* session creation, plus reuse the already-written `prompt.txt` when `.fleet/ticket/<ID>/` was present. Both re-asked the original task on every session after the first, and the reuse branch never looked at the branch at all, so a worktree that once held a ticket kept seeding it after the checkout had moved on to `master`. A seeded first message is the *gesture* of starting a worktree from a ticket, not a property of the directory. `TestTicketWorkStaysOffTheWorkers` keeps it out of `refreshAllGitAndPR`, whose `workerStallThreshold` (90s) is already sized against ~70s of git + `gh` per repo. This is only affordable because there is no badge, hence no live state to keep fresh. - The one mutation resolves the team's started state by **type**, against a position-sorted list (`TestStartedStateResolvesByTypeAndPosition`), so it works on a team whose started state is called "In Dev" or "Doing". Position matters as much as type: a real team has several started states (In Progress at position 2, In Review at 1002) and the lowest is what a human means by "I'm starting this" — any other choice would move a fresh ticket straight to review. Fires **only on create-from-ticket** (config `linear_ticket_start`, default true), never when a later session opens in an existing worktree — by then a human may have moved the issue on, and dragging it backwards is the worst thing this could do. `meta.json` records `state_write` so it stays exactly-once. - The seeded prompt is a **short pointer that tells the agent not to start** (`TestSeedPromptTellsAgentNotToStart`), stated at the top and bottom because a first message describing a task reads as an instruction to perform it. Line 1 leads with the identifier before the title because it is three surfaces at once: the agent's instruction, the preview pane's prompt strip, and the input to `naming.GenerateTitle`, which cuts at ~50 runes. It rides `sessionCreateMsg.prompt` -> `Session.InitialPrompt`. -- **Base branch autocomplete** (`internal/ui/workspace_picker_branch.go`, issue #277): the `w` dialog's Base branch field suggests branches beneath itself, on the ticket rows' vocabulary (`▸` + `selTitle`, dim otherwise, `branchMaxRows`=5) and through the same `setSelection` highlight. The list is fetched **once**, by `fetchWorkspaceListForRepo` on the worker goroutine beside the `GetDefaultBranch` it already ran, and rides `workspaceListMsg.branches` — so filtering is a synchronous `strings.Contains`, with **none** of the ticket machinery's debounce or generation guard, which exist only because a lookup is a network round trip. A failed listing leaves the field exactly as it was. Rows render **only while that field carries the highlight**: the field always holds text, so rendering them unconditionally would park five rows mid-dialog for every user including everyone who never touches the base branch — deliberately unlike `renderTicketBlock`, which renders whenever tickets exist because prose in the *other* field is what put them there. -- **`tab` moves between fields, `↓`/`↑` walk rows** — split apart for this. They were one merged case, which was fine while only the New branch field grew rows and stopped being fine the moment the Base branch field grew its own: tabbing off a focused base field would have taken six presses to reach the next input. The `↓`/`↑` walk is the full continuous path (base input → base rows → new-branch input → ticket rows → worktree list) and must retrace itself exactly — which is why `↑` from the New branch field lands on the *last* base row (`visibleBranchCount()-1`), not the base input. Cost: `tab` no longer walks the worktree list; `↓` still does. `visibleBranchCount()` is deliberately **not** focus-gated even though `renderBranchBlock` is — that `↑` handler asks for the last row while focus is still elsewhere, and a focus gate would answer 0 and silently land on the input. +- **Base branch autocomplete** (`internal/ui/workspace_picker_branch.go`, issue #277): the `w` dialog's Base branch field suggests branches beneath itself, on the ticket rows' vocabulary (`▸` + `selTitle`, dim otherwise, `branchMaxRows`=5) and through the same `setSelection` highlight. The list is fetched **once**, by `fetchWorkspaceListForRepo` on the worker goroutine beside the `GetDefaultBranch` it already ran, and rides `workspaceListMsg.branches` — so filtering is a synchronous `strings.Contains`, with **none** of the ticket machinery's debounce or generation guard, which exist only because a lookup is a network round trip. A failed listing leaves the field exactly as it was. While that field carries the highlight the **existing-worktrees list is hidden**, which is a height budget rather than a flourish: the dialog has none of its own (`wrapDialog` only `Place`s, and the worktree loop is unbounded), so at 80×24 with six worktrees five suggestion rows took the box from 21 lines to 26 and pushed the footer off the bottom. Dropping the list reclaims more than the rows add, so the focused dialog is strictly *shorter* than the resting one (`TestWorktreeFocusedDialogIsNoTallerThanAtRest`; `TestWorktreeDialogRowsNeverOverflow` is width-only and runs at height 40, so it could never catch this). Capping the rows against remaining height was the alternative and was rejected: `setSelection` runs nowhere near `View`, so the cursor clamp would have started varying with window size. Hidden is not unreachable — `shift+tab` still cycles onto the list and brings it back. Rows render **only while that field carries the highlight**: the field always holds text, so rendering them unconditionally would park five rows mid-dialog for every user including everyone who never touches the base branch — deliberately unlike `renderTicketBlock`, which renders whenever tickets exist because prose in the *other* field is what put them there. +- **`tab` moves between fields, `↓`/`↑` walk rows** — split apart for this. They were one merged case, which was fine while only the New branch field grew rows and stopped being fine the moment the Base branch field grew its own: tabbing off a focused base field would have taken six presses to reach the next input. The `↓`/`↑` walk is the full continuous path (base input → base rows → new-branch input → ticket rows → worktree list) and must retrace itself exactly — which is why `↑` from the New branch field lands on the *last* base row (`visibleBranchCount()-1`), not the base input. It **cycles**: `tab` wraps the worktree list back to the base field and `shift+tab` wraps the other way, because a key documented as "next field" that does nothing on the *last* field is a dead key rather than a design, and wrapping one direction only moves the dead end onto the other. Cost: `tab` no longer walks the worktree list; `↓` still does. `visibleBranchCount()` is deliberately **not** focus-gated even though `renderBranchBlock` is — that `↑` handler asks for the last row while focus is still elsewhere, and a focus gate would answer 0 and silently land on the input. - **A suggestion is stored as the exact ref that will be written into the field**, which is why a remote-only branch always comes out `origin/`-prefixed (`baseRefFor`). `GitWorktreeProvider.Create` runs `git worktree add <path> -b <new> <base>`, and with `-b` present git resolves `<base>` as a plain revision with **no** remote-tracking DWIM — so a bare remote-only name is a row that looks valid and fails on Enter, and the no-`-b` retry silently drops the base entirely. A field already reading `origin/…` keeps its prefix and matches only branches with a remote (`git.BranchInfo.HasRemote`, added for this): `GetDefaultBranch` pre-fills `origin/<default>` precisely so a worktree starts from the remote tip, and quietly swapping that for the local branch would change what gets built without saying so. `HasRemote` forced `ListBranches` into two passes — a branch shares its committer date with its `origin/` counterpart only when it is *level*, so a branch you have not pulled has its remote ref sort **first**, which the old single-pass form both missed the flag on and emitted twice (`TestListBranchesRemoteCounterparts`; the duplicate was already visible in the `b` key's picker). -- **An at-rest field lists the alternatives, not itself.** Filtering the pre-filled `origin/<default>` yields exactly one row echoing the field — a row that answers nothing and no-ops on Enter — so when the whole match set is that single echo, `rebuildBranchMatches` widens to the unfiltered list (keeping `wantRemote`: you asked for a remote ref, so the wider list is the other remote refs). Narrowly gated on **exact** equality with the field, so a partially-typed unique match still filters normally and the list never widens under your cursor mid-word. +- **An at-rest field lists the alternatives, not itself.** Filtering the pre-filled `origin/<default>` yields exactly one row echoing the field — a row that answers nothing and no-ops on Enter — so a field nobody is typing into widens to the unfiltered list instead, keeping `wantRemote` (you asked for a remote ref, so the wider list is the other remote refs) and **excluding the field's own value**, which would otherwise reinstate the very no-op row the widening removes. That exclusion runs *before* the `branchMaxRows` cap: filtering `matchBranches`' result instead silently returns one row fewer than fits. The gate is `baseAtRest` — set by `Show` and `pickBaseBranch`, cleared by the first typed change — and **not** equality between the field and a branch name, which looks equivalent and is not: typing your way to `master-fix` passes *through* `master`, so the equality form widened 1 row to 5 at the `r` and collapsed back at the `-`, and since the dialog is vertically centred that was the whole box jumping four rows mid-word. - **Ticket suggestions live in the `w` dialog's existing New branch field, not a new field and not a mode** (`internal/ui/workspace_picker_ticket.go`). The field IS the literal option, so nothing duplicates it and only one thing ever claims Enter. Two rules make that hold: **exactly one highlight, and the caret lives with it** — arrowing onto a ticket blurs the input, typing returns both and keeps the keystroke (`isTypingKey`, borrowed from the snooze dialog, whose "the highlight is the promise" rule this follows); and **shape decides the default, never a mode** — text matching a team's identifier shape resolves *in place* (`LooksLikeIdentifier`), prose stays literal with tickets one down-arrow below. *In place* means the field **becomes the branch name** (`applyResolvedBranchName` → `BranchNameFor`) while the highlight stays put; it does **not** mean "only record the ticket". That half was missing at first, so typing `BRZ-3217` fetched the ticket, materialized it and named the session after it — and then created a git worktree literally called `BRZ-3217`, breaking the invariant `pickTicket`'s own comment states, that both ways of naming a ticket end up identical. The rewrite is gated on the field still holding **nothing but that identifier**: the generation counter drops a reply a later keystroke invalidated, but it cannot see a *current* reply for a shorter identifier you paused on en route (`BRZ-321` while typing `BRZ-3217`), which would otherwise drop `brz-321-<slug>` under the cursor and let the rest of the typing land on the end of it. The same check leaves a tail you edited by hand alone. The confirmation line reads **`✓ named from BRZ-3217 · <title>`**, not a bare identifier: it renders in the same place the selectable ticket rows do, so on its own it read as a row you might still need to arrow onto when the naming had already happened — and it is the only thing on screen that explains why the field rewrote itself a moment earlier. The wording holds in both states `d.resolved` can be in, including a tail you typed yourself, because `onFieldChanged` drops the resolution the moment the text stops leading with the identifier. `✓` is U+2713, East-Asian-Neutral, so it is always one column — the same width check the priority gauge had to pass. The highlight never moves on its own; a picker that jumps its own selection is the ambiguity coming back through the window. `setSelection` is the single writer of `focus`/`ticketCursor`/`baseCursor`, enforced by `TestWorktreeSelectionMutatorIsTheOnlyWriter` across all three picker files, because a stray write skips the clamp and renders two selection markers. The footer names what Enter will do and changes as the highlight moves. - Lookups are **debounced (250ms) and generation-guarded**, never per keystroke. Without the generation counter, typing `BRZ-3182` then editing to `BRZ-3184` lets the slower first reply overwrite the field with the wrong ticket's branch name, which then becomes a real git branch (`TestWorktreeStaleTicketReplyIgnored`). The counter is **monotonic and never reset to 0**, unlike `ConfirmDialog.scanGen` — that one lives on `Home` and never recycles; a per-dialog counter reset to zero would let a reply from a previous open match a new one. `d.loading` is deliberately not used for ticket lookups: it swallows every key but `esc`. - Degradation is structural: no credential, no team, rejected key, offline, unknown id, or nothing matching — every case leaves Enter working and the dialog usable (`TestWorktreeEnterAlwaysCreates`). `ErrNotConnected` is the resting state for everyone who never connected and is **never surfaced as an error**; `ErrNotAuthenticated` is, because it names a thing to fix. Persistent failures latch (`ticketsOff`) so a broken credential isn't re-spent on every pause. Once the worktree exists nothing may fail the caller: `Materialize` sits beside `copyClaudeSettingsFile` and `CopyConfiguredFiles` and shares their posture, and a failed fetch costs the prompt, never the session. diff --git a/internal/git/git.go b/internal/git/git.go index 0354bb71..a2a95810 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -5,6 +5,7 @@ import ( "fmt" "os/exec" "path/filepath" + "sort" "strconv" "strings" "time" @@ -90,6 +91,9 @@ func ListBranches(repoPath string) ([]BranchInfo, error) { var refs []parsedRef localSet := make(map[string]bool) remoteSet := make(map[string]bool) + // The newest date seen for a name across BOTH namespaces — see the merge + // below for why the surviving ref cannot simply keep its own. + newest := make(map[string]time.Time) lines := strings.Split(strings.TrimSpace(string(output)), "\n") for _, line := range lines { @@ -119,6 +123,9 @@ func ListBranches(repoPath string) ([]BranchInfo, error) { } else { localSet[name] = true } + if commitDate.After(newest[name]) { + newest[name] = commitDate + } refs = append(refs, parsedRef{name: name, remote: remote, commitDate: commitDate, authorEmail: authorEmail}) } @@ -128,15 +135,30 @@ func ListBranches(repoPath string) ([]BranchInfo, error) { continue // already have local version } branches = append(branches, BranchInfo{ - Name: r.name, - IsRemote: r.remote, - IsCurrent: r.name == currentBranch, - HasRemote: remoteSet[r.name], - CommitDate: r.commitDate, + Name: r.name, + IsRemote: r.remote, + IsCurrent: r.name == currentBranch, + HasRemote: remoteSet[r.name], + // The LATER of the two refs, not the survivor's own date. The + // dedupe keeps the local ref, but a branch you have not pulled has + // a local tip older than origin/<name> — and callers that ask for + // the remote form (the worktree dialog's base-branch suggestions do, + // via baseRefFor) would then resolve a commit this date does not + // describe. It is also what the branch sorts on, so an unpulled + // master would sink below fresher topic branches and fall out of any + // top-N list. + CommitDate: newest[r.name], AuthorEmail: r.authorEmail, }) } + // Re-sorted here rather than trusting for-each-ref's order: revising a date + // above invalidates it, and a stable sort keeps the ref order for genuine + // ties. + sort.SliceStable(branches, func(i, j int) bool { + return branches[i].CommitDate.After(branches[j].CommitDate) + }) + // Move current branch to index 0. for i, b := range branches { if b.IsCurrent && i > 0 { diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 5a5989d6..4649de9e 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -176,3 +176,45 @@ func TestListBranchesRemoteCounterparts(t *testing.T) { } } } + +// TestListBranchesReportsTheLaterOfTheTwoRefs is the half the flags test misses. +// +// The dedupe keeps the LOCAL ref, but a branch you have not pulled has a local +// tip older than origin/<name>. Reporting the survivor's own date makes the row +// describe a different commit than the ref a caller asking for the remote form +// resolves — and since the date is the sort key, an unpulled branch sinks below +// fresher ones and drops out of any top-N list. `lagging` is exactly that shape: +// local at 2020, origin/lagging at 2024. +func TestListBranchesReportsTheLaterOfTheTwoRefs(t *testing.T) { + repo := initBranchRepo(t) + + branches, err := ListBranches(repo) + if err != nil { + t.Fatalf("ListBranches: %v", err) + } + + pos := make(map[string]int, len(branches)) + date := make(map[string]string, len(branches)) + for i, b := range branches { + pos[b.Name] = i + date[b.Name] = b.CommitDate.UTC().Format("2006-01-02") + } + + if got := date["lagging"]; got != "2024-01-01" { + t.Errorf("lagging date = %s, want 2024-01-01 — origin/lagging is the later ref, and it is "+ + "the one baseRefFor hands to git when the field carries the prefix", got) + } + if got := date["level"]; got != "2020-01-01" { + t.Errorf("level date = %s, want 2020-01-01 — its refs are level, so there is nothing to take the max of", got) + } + if got := date["solo"]; got != "2020-01-01" { + t.Errorf("solo date = %s, want 2020-01-01 — no remote counterpart to consider", got) + } + + // Ordering follows the revised dates, or the fix would be cosmetic: `lagging` + // must sit with the 2024 refs, not below `ghost` where its local tip put it. + if pos["lagging"] > pos["level"] || pos["lagging"] > pos["solo"] { + t.Errorf("lagging at %d sorts below level (%d) / solo (%d) — the list must be ordered by the "+ + "date actually reported", pos["lagging"], pos["level"], pos["solo"]) + } +} diff --git a/internal/ui/app.go b/internal/ui/app.go index 75e4d072..a7e69f8e 100644 --- a/internal/ui/app.go +++ b/internal/ui/app.go @@ -7186,6 +7186,8 @@ func (h *Home) fetchWorkspaceListForRepo(repoPath string) tea.Cmd { // else names snowball (issue #168). Custom shell providers own their own // naming and run relative to the selected repo, so leave them untouched. var originKey string + var defaultBranch string + var branches []git.BranchInfo if !provider.IsCustom() { repoPath = git.GetMainWorktreePath(repoPath) // The normalized main-clone path may not be a tracked gitInfoCache key @@ -7194,9 +7196,19 @@ func (h *Home) fetchWorkspaceListForRepo(repoPath string) tea.Cmd { // origin here (worker goroutine, blocking git is fine) so the handler // can seed the cache. originKey = git.GetOriginKey(repoPath) + // These two are read only by the worktree dialog, and the custom + // path never reaches it — workspaceListMsg's handler returns straight + // to createWorkspaceDialog. Outside the guard they were two + // subprocesses the user waited on behind the spinner on every `w`, + // against a directory a shell provider need not even keep in git. + defaultBranch = git.GetDefaultBranch(repoPath) + // Allowed to fail quietly, unlike the worktree list: no branches + // simply means the Base branch field has no suggestions, never a + // dialog that refuses to open. ListBranches already returns nil on + // error, so the failure needs no separate handling. + branches, _ = git.ListBranches(repoPath) } workspaces, err := provider.List(repoPath) - defaultBranch := git.GetDefaultBranch(repoPath) // Resolved here, on the worker goroutine, so the dialog never touches the // filesystem from Update(). Empty when nothing is connected or the repo // names no Linear team, which makes every ticket surface in the dialog @@ -7205,13 +7217,6 @@ func (h *Home) fetchWorkspaceListForRepo(repoPath string) tea.Cmd { if linear.Available() { linearTeams = linear.TeamKeys(repoPath) } - // Same goroutine, same reason — and unlike the worktree list this one is - // allowed to fail quietly: no branches simply means the Base branch field - // has no suggestions, never a dialog that refuses to open. - branches, branchErr := git.ListBranches(repoPath) - if branchErr != nil { - branches = nil - } return workspaceListMsg{ workspaces: workspaces, provider: provider, repoPath: repoPath, defaultBranch: defaultBranch, originKey: originKey, linearTeams: linearTeams, diff --git a/internal/ui/workspace_picker.go b/internal/ui/workspace_picker.go index b95d38b8..b6787d83 100644 --- a/internal/ui/workspace_picker.go +++ b/internal/ui/workspace_picker.go @@ -101,6 +101,12 @@ type WorktreeDialog struct { lastBaseInput string // change detector, so a redraw doesn't refilter + // baseAtRest is true while nobody is typing into the Base branch field — + // set when the dialog opens and when a suggestion is accepted, cleared by + // the first keystroke that changes the text. It is what decides whether the + // rows filter or list the alternatives; see rebuildBranchMatches. + baseAtRest bool + // --- Linear ticket suggestions under the New branch field --- // linearTeams are the team keys this repo tracks. Empty means the whole @@ -165,6 +171,7 @@ func (d *WorktreeDialog) Show(workspaces []workspace.WorkspaceInfo, sessions []* d.branches = branches d.lastBaseInput = defaultBranch + d.baseAtRest = true d.rebuildBranchMatches() d.linearTeams = linearTeams @@ -313,6 +320,11 @@ func (d *WorktreeDialog) Update(msg tea.Msg) (*WorktreeDialog, tea.Cmd) { // and stopped being fine once the Base branch field grew its own: tabbing off // a focused base field would have taken six presses to reach the next input. // Splitting them gives tab one meaning everywhere in this dialog. + // + // It CYCLES rather than stopping at the ends. "tab no longer walks the + // worktree list" is a deliberate cost, but a key documented as "next field" + // doing literally nothing on the last field is a dead key, not a design — + // and wrapping only one way would just move the dead end to shift+tab. case "tab": switch d.focus { case focusBaseBranch: @@ -320,12 +332,22 @@ func (d *WorktreeDialog) Update(msg tea.Msg) (*WorktreeDialog, tea.Cmd) { case focusNewBranch: if len(d.workspaces) > 0 { d.setSelection(focusWorktreeList, 0) + } else { + d.setSelection(focusBaseBranch, baseOnInput) } + case focusWorktreeList: + d.setSelection(focusBaseBranch, baseOnInput) } return d, nil case "shift+tab": switch d.focus { + case focusBaseBranch: + if len(d.workspaces) > 0 { + d.setSelection(focusWorktreeList, 0) + } else { + d.setSelection(focusNewBranch, ticketOnInput) + } case focusNewBranch: d.setSelection(focusBaseBranch, baseOnInput) case focusWorktreeList: @@ -455,6 +477,7 @@ func (d *WorktreeDialog) routeToInput(msg tea.Msg) (*WorktreeDialog, tea.Cmd) { // no debounce and no generation guard. if current := d.baseBranchInput.Value(); current != d.lastBaseInput { d.lastBaseInput = current + d.baseAtRest = false d.rebuildBranchMatches() // Only when the list actually shrank out from under the highlight. // Re-selecting unconditionally would Blur/Focus the input on every @@ -545,8 +568,16 @@ func (d *WorktreeDialog) View() string { b.WriteString("\n") } - // Existing worktrees. - if len(d.workspaces) > 0 { + // Existing worktrees — hidden while the Base branch field carries the + // highlight. Not cosmetic: the dialog has no height budget (wrapDialog only + // Places, and this loop is unbounded), and measured at 80x24 with six + // worktrees the box went 21 rows at rest to 26 focused, taking the footer + // off the bottom. Dropping the list reclaims more than the suggestions add, + // so the focused dialog is strictly shorter than the resting one — a cap on + // the rows would instead have made the cursor clamp depend on terminal + // height, since setSelection runs nowhere near View. The list is only hidden, + // never unreachable: shift+tab still cycles onto it and brings it back. + if len(d.workspaces) > 0 && d.focus != focusBaseBranch { b.WriteString("\n") b.WriteString(DimStyle.Render("Existing worktrees:")) b.WriteString("\n") diff --git a/internal/ui/workspace_picker_branch.go b/internal/ui/workspace_picker_branch.go index 5a38ef78..e54cc797 100644 --- a/internal/ui/workspace_picker_branch.go +++ b/internal/ui/workspace_picker_branch.go @@ -54,27 +54,38 @@ func (d *WorktreeDialog) rebuildBranchMatches() { wantRemote := strings.HasPrefix(text, remotePrefix) q := strings.ToLower(strings.TrimPrefix(text, remotePrefix)) - d.branchMatches = d.matchBranches(q, wantRemote) - - // A field sitting on a complete ref is at rest, and the one row echoing it - // back answers a question nobody asked — Enter on it is a no-op. The useful - // question there is "what else is there", so fall back to the unfiltered - // list. This matters because the field is PRE-FILLED with origin/<default>: - // without it, focusing the field for the first time shows a single row - // repeating what is already on the line above it, and browsing would mean - // clearing a field you probably wanted to keep. - if len(d.branchMatches) == 1 && d.branchMatches[0] == text { + // A field nobody is typing into is at rest, and filtering it against itself + // yields one row echoing the line above — a row that answers nothing and + // no-ops on Enter. The useful question there is "what else is there", so an + // at-rest field lists the alternatives instead. This matters because the + // field is PRE-FILLED with origin/<default>: without it, focusing the field + // for the first time shows that single echo, and browsing would mean + // clearing a value you probably wanted to keep. + // + // Gated on d.baseAtRest, NOT on the text happening to equal a branch name. + // Those look equivalent and are not: typing your way to `master-fix` passes + // through `master`, which IS a complete branch name, so the equality form + // widened 1 row to 5 at the `r` and collapsed back at the `-` — and since + // the dialog is vertically centred, the whole box jumped four rows mid-word. + if d.baseAtRest { // wantRemote is deliberately kept: you asked for a remote ref, so the - // wider list is the other remote refs. - d.branchMatches = d.matchBranches("", wantRemote) + // wider list is the other remote refs. text is excluded because it is + // the value already in the field — offering it back is the same + // no-op row the widening exists to get rid of. + d.branchMatches = d.matchBranches("", wantRemote, text) + return } + d.branchMatches = d.matchBranches(q, wantRemote, "") } // matchBranches returns the refs whose branch name contains q, in the form each -// one takes as a base ref. An empty q matches everything, so an at-rest field -// lists the most recently committed branches — ListBranches is already in that -// order. -func (d *WorktreeDialog) matchBranches(q string, wantRemote bool) []string { +// one takes as a base ref, skipping exclude. An empty q matches everything, so +// an at-rest field lists the most recently committed branches — ListBranches is +// already in that order. +// +// exclude is applied BEFORE the row cap, not to its result: filtering afterwards +// would quietly return one row fewer than the cap allows. +func (d *WorktreeDialog) matchBranches(q string, wantRemote bool, exclude string) []string { var out []string for _, b := range d.branches { if wantRemote && !b.HasRemote { @@ -83,7 +94,11 @@ func (d *WorktreeDialog) matchBranches(q string, wantRemote bool) []string { if q != "" && !strings.Contains(strings.ToLower(b.Name), q) { continue } - out = append(out, baseRefFor(b, wantRemote)) + ref := baseRefFor(b, wantRemote) + if exclude != "" && ref == exclude { + continue + } + out = append(out, ref) if len(out) >= branchMaxRows { break } @@ -108,6 +123,9 @@ func (d *WorktreeDialog) pickBaseBranch(ref string) { // Write the value, then the change detector, or routeToInput refilters // against text it already knows about. d.lastBaseInput = ref + // An accepted ref is a settled field, exactly like a freshly opened one, so + // the list goes back to showing the alternatives rather than this ref alone. + d.baseAtRest = true d.rebuildBranchMatches() d.err = "" d.setSelection(focusBaseBranch, baseOnInput) diff --git a/internal/ui/workspace_picker_branch_test.go b/internal/ui/workspace_picker_branch_test.go index cd55cb68..d16931ae 100644 --- a/internal/ui/workspace_picker_branch_test.go +++ b/internal/ui/workspace_picker_branch_test.go @@ -41,6 +41,26 @@ func (d *WorktreeDialog) send(msg tea.Msg) *WorktreeDialog { return out } +// typeBase replaces the Base branch field's text the way a user does — every +// rune through Update, so routeToInput's change detector runs and the field +// stops being at rest. +// +// Poking SetValue + rebuildBranchMatches instead leaves baseAtRest true from +// Show, so the assertions land on the at-rest branch while claiming to test +// filtering. Three tests here did exactly that and passed for the wrong reason. +func typeBase(t *testing.T, d *WorktreeDialog, text string) *WorktreeDialog { + t.Helper() + d.setSelection(focusBaseBranch, baseOnInput) + d.baseBranchInput.SetValue("") + for _, r := range text { + d = d.send(tea.KeyPressMsg{Code: r, Text: string(r)}) + } + if d.baseAtRest { + t.Fatalf("typing %q left the field at rest — the change detector did not run", text) + } + return d +} + // TestBaseBranchSuggestionsMatchOnName: the ordinary case — type a fragment, // get the branches containing it, in the form that goes into the field. func TestBaseBranchSuggestionsMatchOnName(t *testing.T) { @@ -72,8 +92,7 @@ func TestBaseBranchSuggestionsMatchOnName(t *testing.T) { // looking perfectly valid on screen. func TestBaseBranchRemoteOnlyIsAlwaysPrefixed(t *testing.T) { d := branchDialog(t, branchFixture()...) - d.baseBranchInput.SetValue("remote-only") - d.rebuildBranchMatches() + d = typeBase(t, d, "remote-only") want := []string{"origin/feat/remote-only"} if got := d.branchMatches; !equalStrings(got, want) { @@ -87,8 +106,7 @@ func TestBaseBranchRemoteOnlyIsAlwaysPrefixed(t *testing.T) { func TestBaseBranchOriginPrefixIsPreserved(t *testing.T) { d := branchDialog(t, branchFixture()...) - d.baseBranchInput.SetValue("origin/") - d.rebuildBranchMatches() + d = typeBase(t, d, "origin/") for _, ref := range d.branchMatches { if !strings.HasPrefix(ref, "origin/") { t.Errorf("origin/ query offered %q — the prefix the user typed must survive", ref) @@ -101,11 +119,8 @@ func TestBaseBranchOriginPrefixIsPreserved(t *testing.T) { } } - // Without the prefix, the same branch comes back in its local form. Typed - // partially so the at-rest widening (see TestBaseBranchAtRestFieldListsTheRest) - // stays out of the way — this is about the prefix, not the list length. - d.baseBranchInput.SetValue("mast") - d.rebuildBranchMatches() + // Without the prefix, the same branch comes back in its local form. + d = typeBase(t, d, "mast") if got := d.branchMatches; !equalStrings(got, []string{"master"}) { t.Errorf("bare query = %v, want [master] — no prefix asked for, none added", got) } @@ -129,12 +144,10 @@ func TestBaseBranchAtRestFieldListsTheRest(t *testing.T) { } } - // A partial match that is NOT the whole field still filters normally: the - // widening is for the at-rest case only, not a general fallback. - d.baseBranchInput.SetValue("mast") - d.rebuildBranchMatches() + // Typing filters, whatever the text happens to be. + d = typeBase(t, d, "mast") if got := d.branchMatches; !equalStrings(got, []string{"master"}) { - t.Errorf("partial query = %v, want [master] — a unique match must not widen", got) + t.Errorf("typed query = %v, want [master] — a field being typed into must filter", got) } } @@ -142,8 +155,8 @@ func TestBaseBranchAtRestFieldListsTheRest(t *testing.T) { // dead end. func TestBaseBranchEmptyFieldListsRecent(t *testing.T) { d := branchDialog(t, branchFixture()...) - d.baseBranchInput.SetValue("") - d.rebuildBranchMatches() + d = typeBase(t, d, "x") + d = d.send(tea.KeyPressMsg{Code: tea.KeyBackspace}) if len(d.branchMatches) != len(branchFixture()) { t.Errorf("empty field matched %d branches, want all %d", len(d.branchMatches), len(branchFixture())) } @@ -231,8 +244,8 @@ func TestBaseBranchEnterFillsAndDoesNotCreate(t *testing.T) { // that returned you to it. func TestBaseBranchTypingFromARowKeepsTheKeystroke(t *testing.T) { d := branchDialog(t, branchFixture()...) - d.baseBranchInput.SetValue("") - d.rebuildBranchMatches() + d = typeBase(t, d, "x") + d = d.send(tea.KeyPressMsg{Code: tea.KeyBackspace}) d.setSelection(focusBaseBranch, 0) d = d.send(key("m")) @@ -295,3 +308,191 @@ func equalStrings(a, b []string) bool { } return true } + +// TestBaseBranchWidenedListExcludesTheField — the widened list exists because a +// row echoing the field is a no-op on Enter, so putting that same row back at +// position 0 reintroduces exactly what it was there to remove. +func TestBaseBranchWidenedListExcludesTheField(t *testing.T) { + d := branchDialog(t, branchFixture()...) + d.setSelection(focusBaseBranch, baseOnInput) + + if len(d.branchMatches) == 0 { + t.Fatal("no rows at rest, the test proves nothing") + } + for _, ref := range d.branchMatches { + if ref == d.baseBranchInput.Value() { + t.Errorf("widened list offered %q, the value already in the field — ⏎ on it does nothing", ref) + } + } +} + +// TestBaseBranchExclusionRunsBeforeTheCap: filtering the field's own value out +// of matchBranches' RESULT would quietly return four rows where five fit. +func TestBaseBranchExclusionRunsBeforeTheCap(t *testing.T) { + // The excluded branch sorts first, so a post-filter would lose a row. + branches := []git.BranchInfo{ + {Name: "master", HasRemote: true}, + {Name: "b1", HasRemote: true}, {Name: "b2", HasRemote: true}, + {Name: "b3", HasRemote: true}, {Name: "b4", HasRemote: true}, + {Name: "b5", HasRemote: true}, + } + d := branchDialog(t, branches...) + d.setSelection(focusBaseBranch, baseOnInput) + + if got := len(d.branchMatches); got != branchMaxRows { + t.Errorf("widened list has %d rows, want %d — the exclusion must be applied before the cap, not to its result", + got, branchMaxRows) + } +} + +// TestBaseBranchTypingNeverWidens is the gate the doc sentence describes. +// +// Equality with the field was the wrong test for "at rest": typing toward +// master-fix passes THROUGH master, a complete branch name, so the list went +// 1 row → 5 → 0 across two keystrokes. The dialog is vertically centred, so +// that is the whole box jumping four rows mid-word. +func TestBaseBranchTypingNeverWidens(t *testing.T) { + branches := []git.BranchInfo{ + {Name: "master"}, {Name: "topic-a"}, {Name: "topic-b"}, + {Name: "topic-c"}, {Name: "topic-d"}, + } + d := branchDialog(t, branches...) + + for _, step := range []struct { + typed string + want int + }{ + {"mast", 1}, {"maste", 1}, {"master", 1}, {"master-", 0}, {"master-f", 0}, + } { + d = typeBase(t, d, step.typed) + if got := len(d.branchMatches); got != step.want { + t.Errorf("typed %q -> %d rows %v, want %d", step.typed, got, d.branchMatches, step.want) + } + } +} + +// TestBaseBranchSettlesOnOpenAndOnPick: the flip side — the two ways a field +// arrives at a value without being typed into must both widen. +func TestBaseBranchSettlesOnOpenAndOnPick(t *testing.T) { + d := branchDialog(t, branchFixture()...) + if !d.baseAtRest { + t.Error("a freshly opened dialog is not at rest") + } + if len(d.branchMatches) < 2 { + t.Errorf("opened with %v, want the alternatives listed", d.branchMatches) + } + + d = typeBase(t, d, "base") + if d.baseAtRest { + t.Fatal("typing left the field at rest") + } + d.setSelection(focusBaseBranch, 0) + d, _ = d.Update(keyEnter) + + if !d.baseAtRest { + t.Error("accepting a suggestion left the field un-settled — it is no longer being typed into") + } + for _, ref := range d.branchMatches { + if ref == d.baseBranchInput.Value() { + t.Errorf("after accepting, the list offers %q back", ref) + } + } +} + +// TestWorktreeTabCyclesBothWays: tab is documented as "next field", so a tab +// that does nothing on the last field is a dead key. Wrapping only forward +// would just move the dead end onto shift+tab. +func TestWorktreeTabCyclesBothWays(t *testing.T) { + d := branchDialog(t, branchFixture()...) + d.workspaces = []workspace.WorkspaceInfo{{Name: "wt-a", Path: "/a"}} + + order := []worktreeFocus{focusBaseBranch, focusNewBranch, focusWorktreeList} + d.setSelection(focusBaseBranch, baseOnInput) + for i := 1; i <= len(order); i++ { // one extra step, to land back on the start + d = d.send(keyTab) + if want := order[i%len(order)]; d.focus != want { + t.Fatalf("tab #%d landed on focus=%v, want %v", i, d.focus, want) + } + } + for i := len(order) - 1; i >= 0; i-- { + d = d.send(keyShiftTab) + if want := order[i]; d.focus != want { + t.Fatalf("shift+tab back to %d landed on focus=%v, want %v", i, d.focus, want) + } + } + + // With no worktrees the cycle is two fields, and must still close. + d2 := branchDialog(t, branchFixture()...) + d2.setSelection(focusNewBranch, ticketOnInput) + d2 = d2.send(keyTab) + if d2.focus != focusBaseBranch { + t.Errorf("tab off the last field with no worktrees landed on %v, want the base field", d2.focus) + } +} + +// TestWorktreeFocusedDialogIsNoTallerThanAtRest. +// +// The dialog has no height budget — wrapDialog only Places, and the worktree +// list loop is unbounded — so five suggestion rows took an 80x24 terminal from +// 21 box lines to 26, pushing the footer (which names what Enter does) off the +// bottom mid-interaction. Hiding the worktree list while the base field has the +// highlight reclaims more than the rows add. +// +// TestWorktreeDialogRowsNeverOverflow cannot catch this: it is width-only and +// runs at height 40. +func TestWorktreeFocusedDialogIsNoTallerThanAtRest(t *testing.T) { + var wss []workspace.WorkspaceInfo + for i := 0; i < 6; i++ { + n := "wt-" + string(rune('a'+i)) + wss = append(wss, workspace.WorkspaceInfo{Name: n, Branch: "b", Path: "/" + n}) + } + branches := []git.BranchInfo{ + {Name: "master", HasRemote: true}, {Name: "b1", HasRemote: true}, + {Name: "b2", HasRemote: true}, {Name: "b3", HasRemote: true}, + {Name: "b4", HasRemote: true}, {Name: "b5", HasRemote: true}, + } + + const termH = 24 + boxLines := func(focus worktreeFocus) int { + d := NewWorktreeDialog() + d.SetSize(80, termH) + d.Show(wss, nil, nil, "/r", "origin/master", nil, branches) + d.setSelection(focus, baseOnInput) + n := 0 + for _, l := range strings.Split(d.View(), "\n") { + if strings.ContainsAny(l, "│╭╰") { + n++ + } + } + return n + } + + atRest, focused := boxLines(focusNewBranch), boxLines(focusBaseBranch) + if focused > atRest { + t.Errorf("focusing the base field grew the dialog %d -> %d lines; the suggestions must not cost "+ + "more height than the worktree list they replace", atRest, focused) + } + if focused > termH { + t.Errorf("focused dialog is %d lines on a %d-row terminal — the footer scrolls off", focused, termH) + } +} + +// TestWorktreeListHiddenWhileBaseFocusedStaysReachable: hidden is not gone. The +// list is still in the model, and shift+tab still cycles onto it. +func TestWorktreeListHiddenWhileBaseFocusedStaysReachable(t *testing.T) { + d := branchDialog(t, branchFixture()...) + d.workspaces = []workspace.WorkspaceInfo{{Name: "wt-alpha", Path: "/a", Branch: "b"}} + + d.setSelection(focusBaseBranch, baseOnInput) + if strings.Contains(d.View(), "wt-alpha") { + t.Error("worktree list rendered while the base field has the highlight") + } + + d = d.send(keyShiftTab) + if d.focus != focusWorktreeList { + t.Fatalf("shift+tab from base landed on %v, want the worktree list", d.focus) + } + if !strings.Contains(d.View(), "wt-alpha") { + t.Error("worktree list still hidden after cycling onto it — hidden must not mean unreachable") + } +}