Skip to content

modify: only require submit when changes affect PRs#106

Open
skarim wants to merge 3 commits into
skarim/switch-after-modifyfrom
skarim/modify-only-block-pr-changes
Open

modify: only require submit when changes affect PRs#106
skarim wants to merge 3 commits into
skarim/switch-after-modifyfrom
skarim/modify-only-block-pr-changes

Conversation

@skarim
Copy link
Copy Markdown
Collaborator

@skarim skarim commented May 22, 2026

Previously, gh stack modify always transitioned to PhasePendingSubmit
after completing on any stack with a remote ID (s.ID != ""). This blocked
the user from running another modify until they ran gh stack submit,
even when the modifications only touched local branches without PRs.

This was overly restrictive. If a user is working at the top of their stack
with branches that haven't been pushed or had PRs created yet, restructuring
those branches is a purely local operation — there is no remote state to
reconcile, and no reason to force a submit before allowing further modifies.

What changed

The condition for entering PhasePendingSubmit is now
s.ID != "" && affectsPRs instead of just s.ID != "".

A new affectsPRs flag is tracked throughout the apply process. It is set
to true when any of the following occurs:

  • A renamed branch has a PullRequest ref
  • A folded branch (source or target) has a PullRequest ref
  • A dropped branch has a PullRequest ref
  • A rebased branch (during cascading rebase) has a PullRequest ref

If none of these conditions are met, the modify state file is cleared
immediately — no pending-submit lock, no "run gh stack submit" prompt.

Changes by file

internal/modify/state.go

  • Added AffectsPRs bool field to StateFile. This persists the flag
    across conflict boundaries so that ContinueApply knows whether
    actions applied before the conflict already affected PR branches.

internal/modify/apply.go

  • ApplyPlan: tracks affectsPRs through each step (rename, fold, drop,
    rebase). Saves the flag into conflict state when a conflict occurs.
    Uses s.ID != "" && affectsPRs for the pending-submit decision.
  • ContinueApply: initializes affectsPRs from the saved state file,
    then checks the conflict branch and remaining branches for PRs during
    the cascading rebase. Uses the same combined condition.
  • Both functions set result.NeedsSubmit / show the "run submit" message
    only when the flag is true.

internal/tui/modifyview/types.go

  • Added NeedsSubmit bool to ApplyResult so the caller can use it
    for the success message.

cmd/modify.go

  • printModifySuccess now takes its cue from result.NeedsSubmit
    instead of s.ID != "". The "run gh stack submit" hint is only
    shown when PR branches were actually affected.

internal/modify/apply_test.go

  • Updated TestApplyPlan_PendingSubmitForRemoteStack to use branches
    with PRs and trigger an actual rebase, validating the pending-submit
    path correctly.
  • Added TestApplyPlan_ClearsStateForRemoteStackWithNoPRBranches:
    remote stack where no branches have PRs → state is cleared.
  • Added TestApplyPlan_PendingSubmitOnlyWhenPRBranchesAffected:
    remote stack with a mix of PR and non-PR branches, only the non-PR
    branch is renamed → state is cleared, NeedsSubmit is false.

Behavior summary

Scenario Before After
Modify on local stack (no remote ID) State cleared State cleared (unchanged)
Modify on remote stack, PR branches affected PhasePendingSubmit PhasePendingSubmit (unchanged)
Modify on remote stack, only local branches affected PhasePendingSubmit State cleared ✅

The CheckStateGuard function (used by add, push, sync, unstack,
rebase) already did not block on PhasePendingSubmit, so those commands
are unaffected by this change.


Stack created with GitHub Stacks CLIGive Feedback 💬

@skarim skarim marked this pull request as ready for review May 23, 2026 21:42
Copilot AI review requested due to automatic review settings May 23, 2026 21:42
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adjusts gh stack modify so it only requires a follow-up gh stack submit when the modify operation actually affects branches that have associated PRs, avoiding an unnecessary pending-submit “lock” for purely local-only restructures on a remote-tracked stack.

Changes:

  • Track an affectsPRs flag through modify apply/continue flows and persist it in the modify state file across conflicts.
  • Gate PhasePendingSubmit and the “Run gh stack submit…” success messaging on s.ID != "" && affectsPRs.
  • Update and add tests to cover remote stacks with/without PR branches and conditional pending-submit behavior.
Show a summary per file
File Description
internal/tui/modifyview/types.go Adds NeedsSubmit to ApplyResult so callers can show submit guidance only when needed.
internal/modify/state.go Persists AffectsPRs in the state file to carry PR-impact across conflict boundaries.
internal/modify/apply.go Implements affectsPRs tracking and uses it to decide pending-submit vs clearing state, including --continue handling.
internal/modify/apply_test.go Updates existing pending-submit test and adds new cases for remote stacks with no PR branches / PRs unaffected.
cmd/modify.go Shows the submit hint based on result.NeedsSubmit instead of merely “has remote stack ID”.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

internal/modify/apply.go:822

  • ClearState(gitDir) runs before stack.SaveWithLock(...) in the success path. If SaveWithLock fails, the warning is printed but the state file is already gone, so users lose the recovery handle even though the stack file may not reflect the rewritten refs. Consider clearing the state only after a successful save, or leaving state behind when saving fails so the user can retry/recover.
	// Transition to pending_submit only when PRs are affected
	needsSubmit := s.ID != "" && affectsPRs
	if needsSubmit {
		state.Phase = PhasePendingSubmit
		state.ConflictBranch = ""
		state.RemainingBranches = nil
		state.OriginalRefs = nil
		if err := SaveState(gitDir, state); err != nil {
			cfg.Warningf("failed to update modify state: %s", err)
		}
	} else {
		ClearState(gitDir)
	}
  • Files reviewed: 5/5 changed files
  • Comments generated: 3

Comment thread internal/modify/apply.go Outdated
Comment thread internal/modify/state.go
Comment thread cmd/modify.go
@skarim skarim force-pushed the skarim/switch-after-modify branch from f633f33 to d7f7cab Compare May 23, 2026 22:05
@skarim skarim force-pushed the skarim/modify-only-block-pr-changes branch 2 times, most recently from 7b1c0c7 to 8503b93 Compare May 23, 2026 22:33
@skarim skarim force-pushed the skarim/switch-after-modify branch 2 times, most recently from 2609d7f to 12decbf Compare May 24, 2026 00:36
@skarim skarim force-pushed the skarim/modify-only-block-pr-changes branch from 8503b93 to 81eaf1d Compare May 24, 2026 00:36
@skarim skarim force-pushed the skarim/switch-after-modify branch from 12decbf to f9fbd25 Compare May 24, 2026 00:57
@skarim skarim force-pushed the skarim/modify-only-block-pr-changes branch from 81eaf1d to 75855f3 Compare May 24, 2026 00:57
Previously, `gh stack modify` always transitioned to `PhasePendingSubmit`
after completing on any stack with a remote ID (`s.ID != ""`). This blocked
the user from running another `modify` until they ran `gh stack submit`,
even when the modifications only touched local branches without PRs.

This was overly restrictive. If a user is working at the top of their stack
with branches that haven't been pushed or had PRs created yet, restructuring
those branches is a purely local operation — there is no remote state to
reconcile, and no reason to force a submit before allowing further modifies.

## What changed

The condition for entering `PhasePendingSubmit` is now
`s.ID != "" && affectsPRs` instead of just `s.ID != ""`.

A new `affectsPRs` flag is tracked throughout the apply process. It is set
to `true` when any of the following occurs:

- A **renamed** branch has a `PullRequest` ref
- A **folded** branch (source or target) has a `PullRequest` ref
- A **dropped** branch has a `PullRequest` ref
- A **rebased** branch (during cascading rebase) has a `PullRequest` ref

If none of these conditions are met, the modify state file is cleared
immediately — no pending-submit lock, no "run `gh stack submit`" prompt.

## Changes by file

**`internal/modify/state.go`**
- Added `AffectsPRs bool` field to `StateFile`. This persists the flag
  across conflict boundaries so that `ContinueApply` knows whether
  actions applied before the conflict already affected PR branches.

**`internal/modify/apply.go`**
- `ApplyPlan`: tracks `affectsPRs` through each step (rename, fold, drop,
  rebase). Saves the flag into conflict state when a conflict occurs.
  Uses `s.ID != "" && affectsPRs` for the pending-submit decision.
- `ContinueApply`: initializes `affectsPRs` from the saved state file,
  then checks the conflict branch and remaining branches for PRs during
  the cascading rebase. Uses the same combined condition.
- Both functions set `result.NeedsSubmit` / show the "run submit" message
  only when the flag is true.

**`internal/tui/modifyview/types.go`**
- Added `NeedsSubmit bool` to `ApplyResult` so the caller can use it
  for the success message.

**`cmd/modify.go`**
- `printModifySuccess` now takes its cue from `result.NeedsSubmit`
  instead of `s.ID != ""`. The "run `gh stack submit`" hint is only
  shown when PR branches were actually affected.

**`internal/modify/apply_test.go`**
- Updated `TestApplyPlan_PendingSubmitForRemoteStack` to use branches
  with PRs and trigger an actual rebase, validating the pending-submit
  path correctly.
- Added `TestApplyPlan_ClearsStateForRemoteStackWithNoPRBranches`:
  remote stack where no branches have PRs → state is cleared.
- Added `TestApplyPlan_PendingSubmitOnlyWhenPRBranchesAffected`:
  remote stack with a mix of PR and non-PR branches, only the non-PR
  branch is renamed → state is cleared, `NeedsSubmit` is false.

## Behavior summary

| Scenario | Before | After |
|---|---|---|
| Modify on local stack (no remote ID) | State cleared | State cleared (unchanged) |
| Modify on remote stack, PR branches affected | `PhasePendingSubmit` | `PhasePendingSubmit` (unchanged) |
| Modify on remote stack, only local branches affected | `PhasePendingSubmit` ❌ | State cleared ✅ |

The `CheckStateGuard` function (used by `add`, `push`, `sync`, `unstack`,
`rebase`) already did not block on `PhasePendingSubmit`, so those commands
are unaffected by this change.
@skarim skarim force-pushed the skarim/modify-only-block-pr-changes branch from 75855f3 to 93adbf8 Compare May 24, 2026 02:07
@skarim skarim force-pushed the skarim/modify-only-block-pr-changes branch from 9b8a530 to c582d34 Compare May 24, 2026 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants