fix: require stage all for partially staged commits (#2063)
This commit is contained in:
parent
41f3289964
commit
84d26fc747
|
|
@ -70,6 +70,7 @@ function buildInputs(overrides: Partial<PrimaryActionInputs> = {}): PrimaryActio
|
|||
return {
|
||||
stagedCount: 1,
|
||||
hasUnstagedChanges: false,
|
||||
hasPartiallyStagedChanges: false,
|
||||
hasMessage: true,
|
||||
hasUnresolvedConflicts: false,
|
||||
isCommitting: false,
|
||||
|
|
|
|||
|
|
@ -88,6 +88,7 @@ function buildInputs(overrides: Partial<PrimaryActionInputs> = {}): PrimaryActio
|
|||
return {
|
||||
stagedCount: 1,
|
||||
hasUnstagedChanges: false,
|
||||
hasPartiallyStagedChanges: false,
|
||||
hasMessage: true,
|
||||
hasUnresolvedConflicts: false,
|
||||
isCommitting: false,
|
||||
|
|
|
|||
|
|
@ -58,6 +58,7 @@ function buildInputs(overrides: Partial<PrimaryActionInputs> = {}): PrimaryActio
|
|||
return {
|
||||
stagedCount: 1,
|
||||
hasUnstagedChanges: false,
|
||||
hasPartiallyStagedChanges: false,
|
||||
hasMessage: true,
|
||||
hasUnresolvedConflicts: false,
|
||||
isCommitting: false,
|
||||
|
|
@ -138,9 +139,9 @@ describe('CommitArea primary action icons', () => {
|
|||
expect(primaryHasIcon(element, CloudUpload)).toBe(true)
|
||||
})
|
||||
|
||||
// Why: a dirty tree with nothing staged surfaces 'Stage Files' as the
|
||||
// Why: a dirty tree with nothing staged surfaces 'Stage All' as the
|
||||
// primary, anchored by a Plus icon to read as an additive bulk action.
|
||||
it('renders a plus icon on a Stage Files primary', () => {
|
||||
it('renders a plus icon on a Stage All primary', () => {
|
||||
const props = baseProps({
|
||||
stagedCount: 0,
|
||||
hasUnstagedChanges: true,
|
||||
|
|
|
|||
|
|
@ -111,6 +111,7 @@ function buildInputs(overrides: Partial<PrimaryActionInputs> = {}): PrimaryActio
|
|||
return {
|
||||
stagedCount: 1,
|
||||
hasUnstagedChanges: false,
|
||||
hasPartiallyStagedChanges: false,
|
||||
hasMessage: true,
|
||||
hasUnresolvedConflicts: false,
|
||||
isCommitting: false,
|
||||
|
|
|
|||
|
|
@ -1243,12 +1243,20 @@ function SourceControlInner(): React.JSX.Element {
|
|||
)
|
||||
|
||||
const hasUnstagedChanges = grouped.unstaged.length > 0 || grouped.untracked.length > 0
|
||||
const hasPartiallyStagedChanges = useMemo(() => {
|
||||
if (grouped.staged.length === 0 || grouped.unstaged.length === 0) {
|
||||
return false
|
||||
}
|
||||
const unstagedPaths = new Set(grouped.unstaged.map((entry) => entry.path))
|
||||
return grouped.staged.some((entry) => unstagedPaths.has(entry.path))
|
||||
}, [grouped.staged, grouped.unstaged])
|
||||
|
||||
const primaryAction: PrimaryAction = useMemo(
|
||||
() =>
|
||||
resolvePrimaryAction({
|
||||
stagedCount: grouped.staged.length,
|
||||
hasUnstagedChanges,
|
||||
hasPartiallyStagedChanges,
|
||||
hasMessage: commitMessage.trim().length > 0,
|
||||
hasUnresolvedConflicts: unresolvedConflicts.length > 0,
|
||||
isCommitting,
|
||||
|
|
@ -1263,6 +1271,7 @@ function SourceControlInner(): React.JSX.Element {
|
|||
commitMessage,
|
||||
grouped.staged.length,
|
||||
hasUnstagedChanges,
|
||||
hasPartiallyStagedChanges,
|
||||
isCommitting,
|
||||
isRemoteOperationActive,
|
||||
inFlightRemoteOpKind,
|
||||
|
|
@ -1279,6 +1288,7 @@ function SourceControlInner(): React.JSX.Element {
|
|||
resolveDropdownItems({
|
||||
stagedCount: grouped.staged.length,
|
||||
hasUnstagedChanges,
|
||||
hasPartiallyStagedChanges,
|
||||
hasMessage: commitMessage.trim().length > 0,
|
||||
hasUnresolvedConflicts: unresolvedConflicts.length > 0,
|
||||
isCommitting,
|
||||
|
|
@ -1293,6 +1303,7 @@ function SourceControlInner(): React.JSX.Element {
|
|||
commitMessage,
|
||||
grouped.staged.length,
|
||||
hasUnstagedChanges,
|
||||
hasPartiallyStagedChanges,
|
||||
isCommitting,
|
||||
isRemoteOperationActive,
|
||||
inFlightRemoteOpKind,
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ function inputs(overrides: Partial<PrimaryActionInputs> = {}): PrimaryActionInpu
|
|||
return {
|
||||
stagedCount: 0,
|
||||
hasUnstagedChanges: false,
|
||||
hasPartiallyStagedChanges: false,
|
||||
hasMessage: false,
|
||||
hasUnresolvedConflicts: false,
|
||||
isCommitting: false,
|
||||
|
|
@ -54,6 +55,25 @@ describe('resolveDropdownItems', () => {
|
|||
expect(byKind.commit_sync.disabled).toBe(true)
|
||||
})
|
||||
|
||||
it('disables commit actions when staged files also have unstaged changes', () => {
|
||||
const items = resolveDropdownItems(
|
||||
inputs({
|
||||
stagedCount: 1,
|
||||
hasUnstagedChanges: true,
|
||||
hasPartiallyStagedChanges: true,
|
||||
hasMessage: true,
|
||||
upstreamStatus: { hasUpstream: true, ahead: 1, behind: 0 }
|
||||
})
|
||||
)
|
||||
const byKind = Object.fromEntries(
|
||||
items.filter((e) => e.kind !== 'separator').map((e) => [e.kind, e])
|
||||
)
|
||||
expect(byKind.commit.disabled).toBe(true)
|
||||
expect(byKind.commit_push.disabled).toBe(true)
|
||||
expect(byKind.commit_sync.disabled).toBe(true)
|
||||
expect(byKind.commit.title).toBe('Stage all changes before committing partially staged files')
|
||||
})
|
||||
|
||||
it('disables push actions but keeps Fetch enabled when branch has no upstream', () => {
|
||||
const items = resolveDropdownItems(
|
||||
inputs({
|
||||
|
|
|
|||
|
|
@ -57,6 +57,7 @@ function formatSyncLabel(base: string, ahead: number, behind: number): string {
|
|||
export function resolveDropdownItems(inputs: PrimaryActionInputs): DropdownEntry[] {
|
||||
const {
|
||||
stagedCount,
|
||||
hasPartiallyStagedChanges,
|
||||
hasMessage,
|
||||
hasUnresolvedConflicts,
|
||||
isCommitting,
|
||||
|
|
@ -94,6 +95,9 @@ export function resolveDropdownItems(inputs: PrimaryActionInputs): DropdownEntry
|
|||
if (!hasStaged) {
|
||||
return 'Stage at least one file to commit'
|
||||
}
|
||||
if (hasPartiallyStagedChanges) {
|
||||
return 'Stage all changes before committing partially staged files'
|
||||
}
|
||||
if (!hasMessage) {
|
||||
return 'Enter a commit message to commit'
|
||||
}
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ function inputs(overrides: Partial<PrimaryActionInputs> = {}): PrimaryActionInpu
|
|||
return {
|
||||
stagedCount: 0,
|
||||
hasUnstagedChanges: false,
|
||||
hasPartiallyStagedChanges: false,
|
||||
hasMessage: false,
|
||||
hasUnresolvedConflicts: false,
|
||||
isCommitting: false,
|
||||
|
|
@ -244,11 +245,11 @@ describe('resolvePrimaryAction', () => {
|
|||
})
|
||||
|
||||
// Why: dirty trees (no staged, has unstaged/untracked) must surface a
|
||||
// 'Stage Files' primary regardless of upstream state. Pulling/syncing on
|
||||
// 'Stage All' primary regardless of upstream state. Pulling/syncing on
|
||||
// a dirty tree fails ("Please commit or stash them"), and pushing skips
|
||||
// the immediate user need (prepare a commit), so the staging rung
|
||||
// intercepts before any remote rung fires.
|
||||
it('returns Stage Files on a dirty tree that is behind upstream', () => {
|
||||
it('returns Stage All on a dirty tree that is behind upstream', () => {
|
||||
const result = resolvePrimaryAction(
|
||||
inputs({
|
||||
hasUnstagedChanges: true,
|
||||
|
|
@ -257,13 +258,13 @@ describe('resolvePrimaryAction', () => {
|
|||
)
|
||||
expect(result).toEqual({
|
||||
kind: 'stage',
|
||||
label: 'Stage Files',
|
||||
label: 'Stage All',
|
||||
title: 'Stage all changes',
|
||||
disabled: false
|
||||
})
|
||||
})
|
||||
|
||||
it('returns Stage Files on a dirty tree that is ahead of upstream', () => {
|
||||
it('returns Stage All on a dirty tree that is ahead of upstream', () => {
|
||||
const result = resolvePrimaryAction(
|
||||
inputs({
|
||||
hasUnstagedChanges: true,
|
||||
|
|
@ -271,11 +272,11 @@ describe('resolvePrimaryAction', () => {
|
|||
})
|
||||
)
|
||||
expect(result.kind).toBe('stage')
|
||||
expect(result.label).toBe('Stage Files')
|
||||
expect(result.label).toBe('Stage All')
|
||||
expect(result.disabled).toBe(false)
|
||||
})
|
||||
|
||||
it('returns Stage Files on a dirty tree with no upstream branch', () => {
|
||||
it('returns Stage All on a dirty tree with no upstream branch', () => {
|
||||
const result = resolvePrimaryAction(
|
||||
inputs({
|
||||
hasUnstagedChanges: true,
|
||||
|
|
@ -285,7 +286,7 @@ describe('resolvePrimaryAction', () => {
|
|||
expect(result.kind).toBe('stage')
|
||||
})
|
||||
|
||||
it('returns Stage Files on a dirty tree while upstream status is still loading', () => {
|
||||
it('returns Stage All on a dirty tree while upstream status is still loading', () => {
|
||||
const result = resolvePrimaryAction(
|
||||
inputs({ hasUnstagedChanges: true, upstreamStatus: undefined })
|
||||
)
|
||||
|
|
@ -293,11 +294,27 @@ describe('resolvePrimaryAction', () => {
|
|||
expect(result.disabled).toBe(false)
|
||||
})
|
||||
|
||||
it('still resolves to Commit when both staged and unstaged exist (staged wins)', () => {
|
||||
it('returns Stage All when a staged file also has unstaged changes', () => {
|
||||
const result = resolvePrimaryAction(
|
||||
inputs({
|
||||
stagedCount: 1,
|
||||
hasUnstagedChanges: true,
|
||||
hasPartiallyStagedChanges: true,
|
||||
hasMessage: true,
|
||||
upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 }
|
||||
})
|
||||
)
|
||||
expect(result.kind).toBe('stage')
|
||||
expect(result.label).toBe('Stage All')
|
||||
expect(result.disabled).toBe(false)
|
||||
})
|
||||
|
||||
it('still resolves to Commit when staged and unrelated unstaged files exist', () => {
|
||||
const result = resolvePrimaryAction(
|
||||
inputs({
|
||||
stagedCount: 1,
|
||||
hasUnstagedChanges: true,
|
||||
hasPartiallyStagedChanges: false,
|
||||
hasMessage: true,
|
||||
upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 }
|
||||
})
|
||||
|
|
@ -315,13 +332,13 @@ describe('resolvePrimaryAction', () => {
|
|||
expect(result.title).toBe('Enter a commit message to commit')
|
||||
})
|
||||
|
||||
it('returns Stage Files when unstaged changes exist on an in-sync branch', () => {
|
||||
it('returns Stage All when unstaged changes exist on an in-sync branch', () => {
|
||||
const result = resolvePrimaryAction(
|
||||
inputs({ hasUnstagedChanges: true, upstreamStatus: upstreamInSync })
|
||||
)
|
||||
expect(result).toEqual({
|
||||
kind: 'stage',
|
||||
label: 'Stage Files',
|
||||
label: 'Stage All',
|
||||
title: 'Stage all changes',
|
||||
disabled: false
|
||||
})
|
||||
|
|
|
|||
|
|
@ -42,6 +42,7 @@ export type PrimaryAction = {
|
|||
export type PrimaryActionInputs = {
|
||||
stagedCount: number
|
||||
hasUnstagedChanges: boolean
|
||||
hasPartiallyStagedChanges: boolean
|
||||
hasMessage: boolean
|
||||
hasUnresolvedConflicts: boolean
|
||||
isCommitting: boolean
|
||||
|
|
@ -58,7 +59,7 @@ export type PrimaryActionInputs = {
|
|||
}
|
||||
|
||||
const PRIMARY_LABEL_BY_KIND: Record<Exclude<PrimaryActionKind, 'commit'>, string> = {
|
||||
stage: 'Stage Files',
|
||||
stage: 'Stage All',
|
||||
push: 'Push',
|
||||
pull: 'Pull',
|
||||
sync: 'Sync',
|
||||
|
|
@ -85,11 +86,13 @@ function describeSyncCounts(ahead: number, behind: number): string {
|
|||
* 1. In-flight commit locks the primary to a disabled "Commit".
|
||||
* 2. In-flight remote operation keeps the current label but disables it.
|
||||
* 3. Unresolved conflicts block the commit path entirely.
|
||||
* 4. Has staged files + message → plain "Commit" (compound flows live in
|
||||
* the dropdown; after the commit lands, step 6 rotates the primary to
|
||||
* 4. Has partially staged files → "Stage All" to avoid hook-time partial
|
||||
* stash conflicts.
|
||||
* 5. Has staged files + message → plain "Commit" (compound flows live in
|
||||
* the dropdown; after the commit lands, step 7 rotates the primary to
|
||||
* the appropriate single remote action).
|
||||
* 5. Has staged files + no message → disabled "Commit" with a reason.
|
||||
* 6. Clean tree → adaptive remote action (or disabled "Commit" no-op).
|
||||
* 6. Has staged files + no message → disabled "Commit" with a reason.
|
||||
* 7. Clean tree → adaptive remote action (or disabled "Commit" no-op).
|
||||
*
|
||||
* An undefined upstream status means fetchUpstreamStatus has not resolved
|
||||
* yet for this worktree. We return a disabled Commit so the button has a
|
||||
|
|
@ -100,6 +103,7 @@ export function resolvePrimaryAction(inputs: PrimaryActionInputs): PrimaryAction
|
|||
const {
|
||||
stagedCount,
|
||||
hasUnstagedChanges,
|
||||
hasPartiallyStagedChanges,
|
||||
hasMessage,
|
||||
hasUnresolvedConflicts,
|
||||
isCommitting,
|
||||
|
|
@ -180,10 +184,22 @@ export function resolvePrimaryAction(inputs: PrimaryActionInputs): PrimaryAction
|
|||
|
||||
const hasStaged = stagedCount > 0
|
||||
|
||||
// 4. Has staged files + message → plain Commit. The primary button never
|
||||
// 4. A path with both staged and unstaged edits can make lint-staged's
|
||||
// partial-stash restore fail after formatters rewrite the staged copy. Push
|
||||
// the user through Stage All first so the index matches the worktree.
|
||||
if (hasStaged && hasPartiallyStagedChanges) {
|
||||
return {
|
||||
kind: 'stage',
|
||||
label: 'Stage All',
|
||||
title: 'Stage all changes before committing partially staged files',
|
||||
disabled: false
|
||||
}
|
||||
}
|
||||
|
||||
// 5. Has staged files + message → plain Commit. The primary button never
|
||||
// compounds ("Commit & Push" etc.) — after the commit lands, the primary
|
||||
// naturally rotates to the appropriate remote action (Push / Sync /
|
||||
// Publish Branch) via step 6 below. Users who want the one-click
|
||||
// Publish Branch) via step 7 below. Users who want the one-click
|
||||
// compound flow can still reach it from the dropdown.
|
||||
if (hasStaged && hasMessage) {
|
||||
return {
|
||||
|
|
@ -194,7 +210,7 @@ export function resolvePrimaryAction(inputs: PrimaryActionInputs): PrimaryAction
|
|||
}
|
||||
}
|
||||
|
||||
// 5. Has staged files but no message — user just needs to type something.
|
||||
// 6. Has staged files but no message — user just needs to type something.
|
||||
if (hasStaged && !hasMessage) {
|
||||
return {
|
||||
kind: 'commit',
|
||||
|
|
@ -204,7 +220,7 @@ export function resolvePrimaryAction(inputs: PrimaryActionInputs): PrimaryAction
|
|||
}
|
||||
}
|
||||
|
||||
// 5b. Nothing staged but local changes exist — surface staging as the
|
||||
// 6b. Nothing staged but local changes exist — surface staging as the
|
||||
// primary so dirty trees don't invite a remote op (pull/sync would fail
|
||||
// with uncommitted changes; push/publish skips the actual user need).
|
||||
// Sits before the upstream-status checks so it works regardless of
|
||||
|
|
@ -212,13 +228,13 @@ export function resolvePrimaryAction(inputs: PrimaryActionInputs): PrimaryAction
|
|||
if (!hasStaged && hasUnstagedChanges) {
|
||||
return {
|
||||
kind: 'stage',
|
||||
label: 'Stage Files',
|
||||
label: 'Stage All',
|
||||
title: 'Stage all changes',
|
||||
disabled: false
|
||||
}
|
||||
}
|
||||
|
||||
// 6. Clean tree + no staged files → adaptive remote action.
|
||||
// 7. Clean tree + no staged files → adaptive remote action.
|
||||
if (!upstreamStatus) {
|
||||
return {
|
||||
kind: 'commit',
|
||||
|
|
|
|||
Loading…
Reference in New Issue