Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

fix(submit): tighten submit output and surface swallowed warnings #1197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jonnii wants to merge 1 commit into jonnii/20260610213308/defer-the-submit-TUI-until-the-submission-phase
base: jonnii/20260610213308/defer-the-submit-TUI-until-the-submission-phase
Choose a base branch
Loading
from jonnii/20260610220955/tighten-submit-output-and-surface-swallowed
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions internal/actions/submit/events.go
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,27 @@ func (PreparingEvent) submitEvent() {}
type BranchPlanEvent struct {
BranchName string
Action string // "create" or "update"
PRNumber *int // existing PR number for updates, nil for creates
IsCurrent bool
Empty bool // branch has no commits relative to its parent
Skipped bool
SkipReason string
}

func (BranchPlanEvent) submitEvent() {}

// BranchWarningEvent surfaces a non-fatal warning raised while submitting a
// branch (e.g. labels or reviewers could not be applied). Warnings must flow
// through the handler rather than direct console output: the interactive
// runner quiets the console while the TUI is active, so direct writes during
// the submission phase are silently dropped.
type BranchWarningEvent struct {
BranchName string
Warning string
}

func (BranchWarningEvent) submitEvent() {}

// SubmissionStartEvent indicates the submission phase is beginning.
type SubmissionStartEvent struct {
Branches []BranchInfo
Expand All @@ -68,10 +82,29 @@ type BranchProgressEvent struct {

func (BranchProgressEvent) submitEvent() {}

// CompletionOutcome classifies how a submit run ended, so handlers can decide
// presentation without string-matching messages.
type CompletionOutcome string

// CompletionOutcome values.
const (
OutcomeComplete CompletionOutcome = "complete" // branches were submitted
OutcomeUpToDate CompletionOutcome = "up-to-date" // nothing needed submitting
OutcomeDryRun CompletionOutcome = "dry-run" // plan reported, nothing submitted
OutcomeCanceled CompletionOutcome = "canceled" // user declined a confirmation
OutcomeNothingToSubmit CompletionOutcome = "nothing-to-submit" // no branches in scope / untracked
OutcomeFailed CompletionOutcome = "failed" // at least one branch failed
)

// CompletionEvent indicates the action has finished.
type CompletionEvent struct {
Success bool
Message string // "All PRs up to date", "Dry run complete", etc.
Outcome CompletionOutcome
Message string // human-readable detail, e.g. "All PRs up to date"
}

// Success reports whether the run ended without failure or cancellation.
func (e CompletionEvent) Success() bool {
return e.Outcome != OutcomeFailed && e.Outcome != OutcomeCanceled
}

func (CompletionEvent) submitEvent() {}
Expand Down
37 changes: 34 additions & 3 deletions internal/actions/submit/planning.go
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@ import (
// prepareBranchesForSubmit prepares submission info for each branch, emitting
// events via handler. When remoteStatuses is provided, planning reuses that
// snapshot; otherwise it asks the engine for lazily batched status so create-only
// dry runs do not read remote state.
func prepareBranchesForSubmit(ctx *app.Context, branches engine.Branches, opts Options, currentBranch string, remoteStatuses engine.BranchRemoteStatuses, handler Handler) ([]Info, error) {
// dry runs do not read remote state. empty flags branches with no commits so
// plan output can surface them.
func prepareBranchesForSubmit(ctx *app.Context, branches engine.Branches, opts Options, currentBranch string, remoteStatuses engine.BranchRemoteStatuses, empty map[string]bool, handler Handler) ([]Info, error) {
submissionInfos := make([]Info, 0, len(branches))
nav := ctx.Navigator()

Expand Down Expand Up @@ -65,7 +66,7 @@ func prepareBranchesForSubmit(ctx *app.Context, branches engine.Branches, opts O
Action: action,
IsCurrent: isCurrent,
Skipped: true,
SkipReason: "skipped, no existing PR",
SkipReason: "no existing PR",
})
continue
}
Expand All @@ -88,6 +89,7 @@ func prepareBranchesForSubmit(ctx *app.Context, branches engine.Branches, opts O
handler.OnEvent(BranchPlanEvent{
BranchName: branchName,
Action: action,
PRNumber: prNumber,
IsCurrent: isCurrent,
Skipped: true,
SkipReason: status.Reason,
Expand Down Expand Up @@ -140,7 +142,9 @@ func prepareBranchesForSubmit(ctx *app.Context, branches engine.Branches, opts O
handler.OnEvent(BranchPlanEvent{
BranchName: branchName,
Action: action,
PRNumber: prNumber,
IsCurrent: isCurrent,
Empty: empty[branchName],
Skipped: false,
})

Expand All @@ -159,6 +163,33 @@ func prepareBranchesForSubmit(ctx *app.Context, branches engine.Branches, opts O
return submissionInfos, nil
}

// confirmPrompt describes what --confirm is about to do in concrete terms.
func confirmPrompt(infos []Info) string {
creates, updates := 0, 0
for _, info := range infos {
if info.Action == actionCreate {
creates++
} else {
updates++
}
}
switch {
case creates > 0 && updates > 0:
return fmt.Sprintf("Create %d and update %d PRs?", creates, updates)
case creates > 0:
return fmt.Sprintf("Create %d %s?", creates, pluralPR(creates))
default:
return fmt.Sprintf("Update %d %s?", updates, pluralPR(updates))
}
}

func pluralPR(count int) string {
if count == 1 {
return "PR"
}
return "PRs"
}

// getBranchesToSubmit returns the list of branches to submit based on options
func getBranchesToSubmit(ctx *app.Context, opts Options) ([]string, error) {
nav := ctx.Navigator()
Expand Down
65 changes: 46 additions & 19 deletions internal/actions/submit/submit.go
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func Action(ctx *app.Context, opts Options, handler Handler) error {
// Non-interactive: inform and exit gracefully
ctx.Output.Info("Branch %s is not tracked by stackit.", branchName)
ctx.Output.Tip("Run 'stackit track' to track this branch, then try again.")
handler.OnEvent(CompletionEvent{Success: true, Message: "Branch not tracked"})
handler.OnEvent(CompletionEvent{Outcome: OutcomeNothingToSubmit, Message: "Branch not tracked"})
return nil
}

Expand All @@ -110,7 +110,7 @@ func Action(ctx *app.Context, opts Options, handler Handler) error {
}
if !shouldTrack {
ctx.Output.Info("Skipping. Use 'stackit track --parent <branch>' for a different parent.")
handler.OnEvent(CompletionEvent{Success: true, Message: "Tracking declined"})
handler.OnEvent(CompletionEvent{Outcome: OutcomeNothingToSubmit, Message: "Tracking declined"})
return nil
}

Expand All @@ -127,7 +127,7 @@ func Action(ctx *app.Context, opts Options, handler Handler) error {
return err
}
if len(branches) == 0 {
handler.OnEvent(CompletionEvent{Success: true, Message: "No branches to submit"})
handler.OnEvent(CompletionEvent{Outcome: OutcomeNothingToSubmit, Message: "No branches to submit"})
return nil
}

Expand Down Expand Up @@ -204,31 +204,58 @@ func Action(ctx *app.Context, opts Options, handler Handler) error {
remoteStatuses = <-remoteStatusCh
}

// Branches with no commits still submit (an empty PR can be a placeholder),
// but the plan flags them and interactive runs confirm first.
emptyMap := eng.BatchIsBranchEmpty(branches)

// Prepare branches for submit (show planning phase with current indicator)
submissionInfos, err := prepareBranchesForSubmit(ctx, branchObjs, opts, currentBranchName, remoteStatuses, handler)
submissionInfos, err := prepareBranchesForSubmit(ctx, branchObjs, opts, currentBranchName, remoteStatuses, emptyMap, handler)
if err != nil {
return fmt.Errorf("failed to prepare branches: %w", err)
}

// Check if we should abort
if opts.DryRun {
handler.OnEvent(CompletionEvent{Success: true, Message: "Dry run complete"})
handler.OnEvent(CompletionEvent{Outcome: OutcomeDryRun, Message: "Dry run complete"})
return nil
}

if len(submissionInfos) == 0 {
handler.OnEvent(CompletionEvent{Success: true, Message: "All PRs up to date"})
handler.OnEvent(CompletionEvent{Outcome: OutcomeUpToDate, Message: "All PRs up to date"})
return nil
}

// Confirm empty branches interactively. --confirm already prompts below
// with the full plan visible, so don't ask twice.
emptyCount := 0
for _, info := range submissionInfos {
if emptyMap[info.BranchName] {
emptyCount++
}
}
if emptyCount > 0 && handler.IsInteractive() && !opts.Confirm {
message := fmt.Sprintf("%d branches have no commits — submit anyway?", emptyCount)
if emptyCount == 1 {
message = "1 branch has no commits — submit anyway?"
}
confirmed, err := handler.Confirm(message, true)
if err != nil {
return fmt.Errorf("confirmation canceled: %w", err)
}
if !confirmed {
handler.OnEvent(CompletionEvent{Outcome: OutcomeCanceled, Message: "Submit canceled"})
return nil
}
}

// Handle interactive confirmation
if opts.Confirm {
confirmed, err := handler.Confirm("Proceed with submit?", true)
confirmed, err := handler.Confirm(confirmPrompt(submissionInfos), true)
if err != nil {
return fmt.Errorf("confirmation canceled: %w", err)
}
if !confirmed {
handler.OnEvent(CompletionEvent{Success: false, Message: "Submit canceled"})
handler.OnEvent(CompletionEvent{Outcome: OutcomeCanceled, Message: "Submit canceled"})
return nil
}
}
Expand Down Expand Up @@ -297,7 +324,7 @@ func Action(ctx *app.Context, opts Options, handler Handler) error {
}

if submitErr != nil {
handler.OnEvent(CompletionEvent{Success: false, Message: "Submit failed"})
handler.OnEvent(CompletionEvent{Outcome: OutcomeFailed, Message: "Submit failed"})
return submitErr
}

Expand All @@ -321,13 +348,13 @@ func Action(ctx *app.Context, opts Options, handler Handler) error {

// Push metadata refs for successfully submitted branches
if err := pushMetadataRefs(ctx, branchObjs); err != nil {
handler.OnEvent(CompletionEvent{Success: false, Message: "Submit failed"})
handler.OnEvent(CompletionEvent{Outcome: OutcomeFailed, Message: "Submit failed"})
return fmt.Errorf("failed to push metadata to remote: %w. Your PRs were created/updated successfully, but metadata sync failed. Run 'st sync' and try submitting again", err)
}

ctx.Logger.Info("submit completed branchCount=%v", len(branches))

handler.OnEvent(CompletionEvent{Success: true, Message: "Submit complete"})
handler.OnEvent(CompletionEvent{Outcome: OutcomeComplete, Message: "Submit complete"})
return nil
}

Expand Down Expand Up @@ -385,9 +412,9 @@ func submitBranch(ctx *app.Context, info Info, opts Options, handler Handler, re
var prURL string
var err error
if info.Action == actionCreate {
prURL, err = createPullRequestQuiet(ctx, info, repoOwner, repoName)
prURL, err = createPullRequestQuiet(ctx, info, repoOwner, repoName, handler)
} else {
prURL, err = updatePullRequestQuiet(ctx, info, opts, repoOwner, repoName)
prURL, err = updatePullRequestQuiet(ctx, info, opts, repoOwner, repoName, handler)
}

if err != nil {
Expand Down Expand Up @@ -477,7 +504,7 @@ func pushSubmittedBranches(ctx *app.Context, opts Options, infos []Info, remote
}

// createPullRequestQuiet creates a new pull request without logging
func createPullRequestQuiet(ctx *app.Context, submissionInfo Info, repoOwner, repoName string) (string, error) {
func createPullRequestQuiet(ctx *app.Context, submissionInfo Info, repoOwner, repoName string, handler Handler) (string, error) {
pr := ctx.PR()
nav := ctx.Navigator()

Expand Down Expand Up @@ -506,9 +533,9 @@ func createPullRequestQuiet(ctx *app.Context, submissionInfo Info, repoOwner, re
return "", fmt.Errorf("failed to create PR for %s: %w", submissionInfo.BranchName, err)
}

// Log any warnings from PR creation (e.g., failed to add labels/assignees)
// Surface any warnings from PR creation (e.g., failed to add labels/assignees)
for _, warning := range prResult.Warnings {
ctx.Output.Warn("%s: %s", submissionInfo.BranchName, warning)
handler.OnEvent(BranchWarningEvent{BranchName: submissionInfo.BranchName, Warning: warning})
}

// Update PR info
Expand All @@ -531,7 +558,7 @@ func createPullRequestQuiet(ctx *app.Context, submissionInfo Info, repoOwner, re
}

// updatePullRequestQuiet updates an existing pull request without logging
func updatePullRequestQuiet(ctx *app.Context, submissionInfo Info, opts Options, repoOwner, repoName string) (string, error) {
func updatePullRequestQuiet(ctx *app.Context, submissionInfo Info, opts Options, repoOwner, repoName string, handler Handler) (string, error) {
pr := ctx.PR()
nav := ctx.Navigator()

Expand Down Expand Up @@ -630,9 +657,9 @@ func updatePullRequestQuiet(ctx *app.Context, submissionInfo Info, opts Options,
return "", fmt.Errorf("failed to update PR for %s: %w", submissionInfo.BranchName, err)
}

// Log any warnings from PR update (e.g., failed to add labels/assignees)
// Surface any warnings from PR update (e.g., failed to add labels/assignees)
for _, warning := range updateWarnings {
ctx.Output.Warn("%s: %s", submissionInfo.BranchName, warning)
handler.OnEvent(BranchWarningEvent{BranchName: submissionInfo.BranchName, Warning: warning})
}

// If the base.sha refresh was needed but failed, keep the previously stored BaseSHA
Expand Down
39 changes: 2 additions & 37 deletions internal/actions/submit/submit_validation.go
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,6 @@ func ValidateBranchesToSubmit(ctx *app.Context, branches []string) error {
return err
}

// Validate no empty branches
if err := validateNoEmptyBranches(branches, ctx); err != nil {
return err
}

// Validate no merged/closed branches
if err := validateNoMergedOrClosedBranches(branches, ctx.Status(), ctx); err != nil {
return err
Expand Down Expand Up @@ -115,7 +110,7 @@ func validateBaseRevisions(branches []string, eng engine.BranchStatus, ctx *app.
switch {
case parentBranch.IsTrunk():
if !branch.IsBranchUpToDate() {
ctx.Output.Info("Note that %s has fallen behind trunk. You may encounter conflicts if you attempt to merge it.",
ctx.Output.Warn("%s is behind trunk and may conflict on merge — run 'stackit sync' and 'stackit restack' to update it.",
output.Branch(branchName, false))
}
case validatedBranches[parentBranchName]:
Expand All @@ -138,36 +133,6 @@ func validateBaseRevisions(branches []string, eng engine.BranchStatus, ctx *app.
return nil
}

// validateNoEmptyBranches checks for empty branches and prompts user if found
func validateNoEmptyBranches(branches []string, runtimeCtx *app.Context) error {
// Resolve emptiness for the whole set in one batched rev-parse rather than a
// diff per branch.
empty := runtimeCtx.Engine.BatchIsBranchEmpty(branches)
emptyBranches := []string{}
for _, branchName := range branches {
if empty[branchName] {
emptyBranches = append(emptyBranches, branchName)
}
}

if len(emptyBranches) == 0 {
return nil
}

hasMultiple := len(emptyBranches) > 1
runtimeCtx.Output.Warn("The following branch%s have no changes:", actions.PluralSuffix("branch", hasMultiple))
for _, b := range emptyBranches {
runtimeCtx.Output.Warn("▸ %s", b)
}
runtimeCtx.Output.Warn("Are you sure you want to submit %s?", actions.PluralIt(hasMultiple))

// For now, we'll allow empty branches (non-interactive mode)
// In interactive mode, we would prompt here
// TODO: Add interactive prompt when needed

return nil
}

// validateNoMergedOrClosedBranches checks for merged/closed PRs and prompts user if found
func validateNoMergedOrClosedBranches(branches []string, eng engine.BranchStatus, ctx *app.Context) error {
mergedOrClosedBranches := []string{}
Expand All @@ -187,11 +152,11 @@ func validateNoMergedOrClosedBranches(branches []string, eng engine.BranchStatus
}

hasMultiple := len(mergedOrClosedBranches) > 1
ctx.Output.Tip("You can use 'stackit sync' to find and delete all merged/closed branches automatically and rebase their children.")
ctx.Output.Warn("PR%s for the following branch%s already been merged or closed:", actions.PluralSuffix("PR", hasMultiple), actions.PluralSuffix("branch", hasMultiple))
for _, b := range mergedOrClosedBranches {
ctx.Output.Warn("▸ %s", b)
}
ctx.Output.Tip("Run 'stackit sync' to delete merged/closed branches and rebase their children.")

// For now, we'll allow creating new PRs (non-interactive mode)
// In interactive mode, we would prompt here
Expand Down
2 changes: 1 addition & 1 deletion internal/api/handlers/submit.go
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func (h *SubmitHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
}
branches = append(branches, br)
case submit.CompletionEvent:
success = e.Success
success = e.Success()
completionMsg = e.Message
}
}
Expand Down
2 changes: 1 addition & 1 deletion internal/cli/stack/submit.go
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ func executeSubmit(cmd *cobra.Command, f *submitFlags) error {
}

// Action is the single source of truth for what to submit. The runner
// starts lazily (on the first stack-display event), so calling Action
// starts lazily (when the submission phase begins), so calling Action
// unconditionally no longer flashes the TUI when there's nothing to do.
runner, handler := NewSubmitUI(ctx.Output, ctx.Logger)
defer runner.Cleanup()
Expand Down
Loading
Loading

AltStyle によって変換されたページ (->オリジナル) /