feat: add base branch selection to interactive checkout wizard#176
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…change
The previous implementation had two OnComplete("repos") callbacks which
silently overwrote each other (map-based storage). This consolidates them
into a single callback that handles branch reset, branch fetching, and
base step updates. Also adds DefaultBranch to BranchFetchResult so the
base step cursor is repositioned to the correct default branch when repos
change.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When creating a new branch from a base, the code unconditionally prepended origin/ to local refs. This fails for branches that only exist locally (no remote tracking branch). Now checks if the remote ref exists first and keeps the local ref as fallback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Main extracted runCheckoutInteractive wrapper. Adapted our base branch changes to flow through the new checkoutInteractiveResult struct and runCheckoutInteractive function signature. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- Add integration test for --base with local-only branch (no remote tracking ref) to cover the RefExists fallback in createWorktreeForBranch - Add SetCursor test for empty options list edge case - Remove tea_debug.log files and add to .gitignore Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
raphi011
left a comment
There was a problem hiding this comment.
PR Review Summary
Overall well-structured PR. The OnComplete("repos") consolidation is a genuine bug fix, SetCursor is clean, and the wizard design properly skips/pre-selects steps.
Critical (2) · Important (4) · Suggestions (3)
See inline comments for details. Additional suggestions that couldn't be attached to diff lines:
Suggestion: Stale comment in checkout_test.go:194 — "Fetch step is conditionally skipped for existing branches" should say "Base step" (there is no Fetch step in the wizard).
Suggestion: Duplicate GetDefaultBranch call in checkout_cmd.go:~656 — fetchBranches callback already calls git.GetDefaultBranch and returns it in BranchFetchResult.DefaultBranch, but lines 651-656 call it again separately. The duplication exists because fetchBranches is a callback for repo switches while the initial value is needed now — a brief comment would explain why.
Suggestion: SkipWhen("base") logic is untested — The core feature contract (base step only shown when creating a new branch) has no test coverage for either skip path (BaseFromCLI=true, or existing branch selected).
| baseRef = "origin/" + baseRef | ||
| remoteRef := "origin/" + baseRef | ||
| if git.RefExists(ctx, gitDir, remoteRef) { | ||
| baseRef = remoteRef |
There was a problem hiding this comment.
Critical: Silent local-ref fallback changes behavior for ALL checkouts, not just wizard
This applies to every wt checkout -b --base invocation. Previously, if origin/develop didn't exist, git would produce a clear error. Now it silently falls back to a potentially stale local ref with no logging.
Failure scenarios:
- User forgot to
fetch→ branches from weeks-old local ref silently - Typo
--base developp→ silently uses localdeveloppif it happens to exist
Suggestion: Add a warning log when falling back:
if git.RefExists(ctx, gitDir, remoteRef) {
baseRef = remoteRef
} else {
l := log.FromContext(ctx)
l.Printf("Warning: %s not found, using local ref %s\n", remoteRef, baseRef)
}Alternatively, consider restricting this fallback to only the wizard code path if it's not intended as a general behavior change.
There was a problem hiding this comment.
Added warning log when falling back to local ref. Also added a companion integration test (TestCheckout_BaseBranch_PrefersRemoteOverLocal) that verifies remote is preferred when both exist.
| // Scenario: User runs `wt checkout -b feature --base develop` where develop | ||
| // only exists locally (no origin/develop). BaseRef config is NOT set to "local". | ||
| // Expected: Branch is created from local develop ref (not origin/develop). | ||
| func TestCheckout_BaseBranch_LocalOnlyFallback(t *testing.T) { |
There was a problem hiding this comment.
Critical: Missing test — verify remote ref still preferred when both exist
This test covers the fallback case (only local ref exists), but there's no test verifying the more important contract: when both origin/develop and local develop exist, the remote should still be preferred.
A future refactor could accidentally flip the preference, causing branches to silently create from stale local refs.
Suggestion: Add a companion test using setupTestRepoWithOrigin where both refs exist with different content, verifying the worktree gets the remote's content.
There was a problem hiding this comment.
Added TestCheckout_BaseBranch_PrefersRemoteOverLocal — creates a repo with origin, pushes develop, adds a remote-only commit, then verifies the worktree gets the remote content.
| } | ||
| } | ||
|
|
||
| func TestCheckoutOptions_IncludesBase(t *testing.T) { |
There was a problem hiding this comment.
Important: These two struct tests provide zero value — consider removing
TestCheckoutOptions_IncludesBase and TestCheckoutWizardParams_IncludesDefaultBranch construct a struct and assert the fields contain the values just assigned. They test that Go struct assignment works. A field rename/removal would produce a compile error anyway (named field syntax).
Also: both are missing t.Parallel() (CLAUDE.md: "All tests MUST use t.Parallel() as first statement").
There was a problem hiding this comment.
Removed both. Added t.Parallel() to the remaining test that was missing it (TestBuildBaseBranchOptions_NoWorktreeDecoration).
|
|
||
| // Step 3: Hooks (only when available and not set via CLI) | ||
| // Step 3: Base branch (only when creating new branch and not set via CLI) | ||
| baseStep := steps.NewFilterableList("base", "Base Branch", "Select a base branch to create from", branchOptions). |
There was a problem hiding this comment.
Important: Base step reuses branch options with "(worktree)" suffix
The base step is initialized with the same branchOptions built by buildBranchOptions(), which appends (worktree) to labels for branches that already have a worktree. For base branch selection ("which branch to fork from"), this decoration is misleading — the user isn't opening a worktree.
Suggestion: Build separate options for the base step without the worktree decoration, or use the Value field (plain branch name) for display.
There was a problem hiding this comment.
Fixed. Added buildBaseBranchOptions() that uses plain branch names (Label == Value == Name). The base step and the OnComplete callback both use it now.
| return s.cursor | ||
| } | ||
|
|
||
| // SetCursor sets the cursor position, clamping to valid bounds. |
There was a problem hiding this comment.
Important: SetCursor semantics ambiguous with "create" option
SetCursor clamps against len(s.filtered) but doesn't account for the +1 offset when shouldShowCreate() is true. Safe today since the base step doesn't use WithCreateFromFilter, but the public API contract is unclear.
Suggestion: Extend the doc comment to clarify that idx refers to the position in the filtered options list, not the visual cursor position (which includes the create option when enabled).
There was a problem hiding this comment.
Extended the doc comment to clarify that idx refers to the filtered options list position, not the visual cursor position (which includes the create option when enabled).
|
Addressing the top-level suggestions: Stale comment in checkout_test.go:194 — Fixed: "Fetch step" → "Base step". Duplicate GetDefaultBranch call — Eliminated. Now uses SkipWhen("base") untested — Noted. The skip logic is simple (check |
- Add warning log when falling back from missing remote ref to local ref - Add integration test verifying remote ref is preferred over local - Remove zero-value struct tests that tested Go assignment - Use plain branch names (no worktree decoration) in base step - Clarify SetCursor doc comment re: create option offset - Fix stale comment: "Fetch step" → "Base step" - Eliminate duplicate GetDefaultBranch call using fetchBranches result Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
wt checkout -iwizard that appears when creating a new branch, allowing users to select which branch to base fromgit symbolic-ref--baseis explicitly passed on CLI or when selecting an existing branchSetCursormethod toFilterableListStepfor programmatic cursor positioningOnComplete("repos")callbacks silently overwrote each other (map-based storage) — consolidated into a single callbackTests added: 4 unit tests