xiaoxue_admin
/
orca
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix: address review findings (#4814)

This commit is contained in:
Jinjing 2026-06-07 11:18:43 -07:00 committed by GitHub
parent de7e32c41c
commit 5cfeaa34c0
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
16 changed files with 236 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
src/main/git/status.test.ts
View File

@ -463,6 +463,32 @@ describe('getStatus', () => {
])
})
it('preserves porcelain v2 submodule dirtiness flags on status rows', async () => {
readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n')
existsSyncMock.mockReturnValue(false)
gitExecFileAsyncMock.mockResolvedValueOnce({
stdout:
'1 AM S..U 000000 160000 160000 0000000000000000000000000000000000000000 7844cb64e631f17a9ca5b548f3500ef7cecd2f17 nested-repo\n'
})
const result = await getStatus('/repo')
expect(result.entries).toEqual([
{
path: 'nested-repo',
status: 'added',
area: 'staged',
submodule: { commitChanged: false, trackedChanges: false, untrackedChanges: true }
},
{
path: 'nested-repo',
status: 'modified',
area: 'unstaged',
submodule: { commitChanged: false, trackedChanges: false, untrackedChanges: true }
}
])
})
it('omits ignored files by default and parses them when requested', async () => {
readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n')
existsSyncMock.mockReturnValue(false)

37
src/main/git/status.ts
View File

@ -138,6 +138,7 @@ export async function getStatus(
// Changed entries: "1 XY sub mH mI mW hH path" or "2 XY sub mH mI mW hH X\tscore\tpath\torigPath"
const parts = line.split(' ')
const xy = parts[1]
const submodule = parseSubmoduleStatus(parts[2])
const indexStatus = xy[0]
const worktreeStatus = xy[1]
@ -149,24 +150,41 @@ export async function getStatus(
const path = decodeGitCQuotedPath(tabParts[0].split(' ').slice(9).join(' '))
const oldPath = decodeGitCQuotedPath(tabParts.slice(1).join('\t'))
if (indexStatus !== '.') {
entries.push({ path, status: parseStatusChar(indexStatus), area: 'staged', oldPath })
entries.push({
path,
status: parseStatusChar(indexStatus),
area: 'staged',
oldPath,
...(submodule ? { submodule } : {})
})
}
if (worktreeStatus !== '.') {
entries.push({
path,
status: parseStatusChar(worktreeStatus),
area: 'unstaged',
oldPath
oldPath,
...(submodule ? { submodule } : {})
})
}
} else {
// Regular change entry
const path = decodeGitCQuotedPath(parts.slice(8).join(' '))
if (indexStatus !== '.') {
entries.push({ path, status: parseStatusChar(indexStatus), area: 'staged' })
entries.push({
path,
status: parseStatusChar(indexStatus),
area: 'staged',
...(submodule ? { submodule } : {})
})
}
if (worktreeStatus !== '.') {
entries.push({ path, status: parseStatusChar(worktreeStatus), area: 'unstaged' })
entries.push({
path,
status: parseStatusChar(worktreeStatus),
area: 'unstaged',
...(submodule ? { submodule } : {})
})
}
}
} else if (line.startsWith('? ')) {
@ -429,6 +447,17 @@ function parseStatusChar(char: string): GitFileStatus {
}
}
function parseSubmoduleStatus(submoduleField: string | undefined): GitStatusEntry['submodule'] {
if (!submoduleField?.startsWith('S')) {
return undefined
}
return {
commitChanged: submoduleField[1] === 'C',
trackedChanges: submoduleField[2] === 'M',
untrackedChanges: submoduleField[3] === 'U'
}
}
function parseBranchStatusChar(char: string): GitBranchChangeStatus {
switch (char) {
case 'M':

21
src/relay/git-handler-utils.test.ts
View File

@ -47,4 +47,25 @@ describe('parseStatusOutput', () => {
{ path: 'src/new name.ts', oldPath: 'src/old name.ts', status: 'renamed', area: 'staged' }
])
})
it('parses submodule dirtiness flags from porcelain records', () => {
const result = parseStatusOutput(
'1 AM S..U 000000 160000 160000 0000000000000000000000000000000000000000 7844cb64e631f17a9ca5b548f3500ef7cecd2f17 nested-repo\n'
)
expect(result.entries).toEqual([
{
path: 'nested-repo',
status: 'added',
area: 'staged',
submodule: { commitChanged: false, trackedChanges: false, untrackedChanges: true }
},
{
path: 'nested-repo',
status: 'modified',
area: 'unstaged',
submodule: { commitChanged: false, trackedChanges: false, untrackedChanges: true }
}
])
})
})

30
src/relay/git-status-output-parser.ts
View File

@ -69,6 +69,7 @@ export function parseStatusOutput(stdout: string): {
if (line.startsWith('1 ') || line.startsWith('2 ')) {
const parts = line.split(' ')
const xy = parts[1]
const submodule = parseSubmoduleStatus(parts[2])
const indexStatus = xy[0]
const worktreeStatus = xy[1]
@ -83,7 +84,8 @@ export function parseStatusOutput(stdout: string): {
path: filePath,
status: parseStatusChar(indexStatus),
area: 'staged',
oldPath
oldPath,
...(submodule ? { submodule } : {})
})
}
if (worktreeStatus !== '.') {
@ -91,19 +93,26 @@ export function parseStatusOutput(stdout: string): {
path: filePath,
status: parseStatusChar(worktreeStatus),
area: 'unstaged',
oldPath
oldPath,
...(submodule ? { submodule } : {})
})
}
} else {
const filePath = parts.slice(8).join(' ')
if (indexStatus !== '.') {
entries.push({ path: filePath, status: parseStatusChar(indexStatus), area: 'staged' })
entries.push({
path: filePath,
status: parseStatusChar(indexStatus),
area: 'staged',
...(submodule ? { submodule } : {})
})
}
if (worktreeStatus !== '.') {
entries.push({
path: filePath,
status: parseStatusChar(worktreeStatus),
area: 'unstaged'
area: 'unstaged',
...(submodule ? { submodule } : {})
})
}
}
@ -133,6 +142,19 @@ export function parseStatusOutput(stdout: string): {
}
}
function parseSubmoduleStatus(
submoduleField: string | undefined
): { commitChanged: boolean; trackedChanges: boolean; untrackedChanges: boolean } | undefined {
if (!submoduleField?.startsWith('S')) {
return undefined
}
return {
commitChanged: submoduleField[1] === 'C',
trackedChanges: submoduleField[2] === 'M',
untrackedChanges: submoduleField[3] === 'U'
}
}
function parseBranchAheadBehind(line: string): { ahead: number; behind: number } | null {
const match = line.match(/^# branch\.ab \+(\d+) -(\d+)$/)
if (!match) {

1
src/renderer/src/components/right-sidebar/CommitArea.chevron-spinner.test.tsx
View File

@ -14,6 +14,7 @@ function buildInputs(overrides: Partial<PrimaryActionInputs> = {}): PrimaryActio
return {
stagedCount: 1,
hasUnstagedChanges: false,
hasStageableChanges: false,
hasPartiallyStagedChanges: false,
hasMessage: true,
hasUnresolvedConflicts: false,

1
src/renderer/src/components/right-sidebar/CommitArea.generate.test.tsx
View File

@ -9,6 +9,7 @@ function buildInputs(overrides: Partial<PrimaryActionInputs> = {}): PrimaryActio
return {
stagedCount: 1,
hasUnstagedChanges: false,
hasStageableChanges: false,
hasPartiallyStagedChanges: false,
hasMessage: true,
hasUnresolvedConflicts: false,

2
src/renderer/src/components/right-sidebar/CommitArea.primary-icons.test.tsx
View File

@ -14,6 +14,7 @@ function buildInputs(overrides: Partial<PrimaryActionInputs> = {}): PrimaryActio
return {
stagedCount: 1,
hasUnstagedChanges: false,
hasStageableChanges: false,
hasPartiallyStagedChanges: false,
hasMessage: true,
hasUnresolvedConflicts: false,
@ -123,6 +124,7 @@ describe('CommitArea primary action icons', () => {
baseProps({
stagedCount: 0,
hasUnstagedChanges: true,
hasStageableChanges: true,
hasMessage: false,
upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 }
})

1
src/renderer/src/components/right-sidebar/CommitArea.test.tsx
View File

@ -9,6 +9,7 @@ function buildInputs(overrides: Partial<PrimaryActionInputs> = {}): PrimaryActio
return {
stagedCount: 1,
hasUnstagedChanges: false,
hasStageableChanges: false,
hasPartiallyStagedChanges: false,
hasMessage: true,
hasUnresolvedConflicts: false,

43
src/renderer/src/components/right-sidebar/SourceControl.tsx
View File

@ -67,6 +67,8 @@ import {
getDiscardAllPaths,
getStageAllPaths,
getUnstageAllPaths,
isStageableStatusEntry,
isSubmoduleWorktreeOnlyChange,
runDiscardAllForArea,
type DiscardAllArea
} from './discard-all-sequence'
@ -259,6 +261,8 @@ const SOURCE_CONTROL_TREE_DIRECTORY_PADDING_PX = 8
const SOURCE_CONTROL_TREE_FILE_PADDING_PX = 20
const EMPTY_GIT_HISTORY_STATE: GitHistoryPanelState = { status: 'idle' }
const DEFAULT_COLLAPSED_SECTIONS = ['history'] as const
const SUBMODULE_WORKTREE_ONLY_LABEL = 'Submodule changes - stage inside submodule'
const SUBMODULE_WORKTREE_ONLY_STAGE_TOOLTIP = 'Stage these changes inside the submodule'
function createDefaultCollapsedSections(): Set<string> {
return new Set(DEFAULT_COLLAPSED_SECTIONS)
@ -2847,19 +2851,28 @@ function SourceControlInner(): React.JSX.Element {
worktreePath
])
const stageableUnstagedPaths = useMemo(
() => [
...getStageAllPaths(grouped.unstaged, 'unstaged'),
...getStageAllPaths(grouped.untracked, 'untracked')
],
[grouped.unstaged, grouped.untracked]
)
const hasUnstagedChanges = grouped.unstaged.length > 0 || grouped.untracked.length > 0
const hasStageableChanges = stageableUnstagedPaths.length > 0
const hasPartiallyStagedChanges = useMemo(() => {
if (grouped.staged.length === 0 || grouped.unstaged.length === 0) {
if (grouped.staged.length === 0 || stageableUnstagedPaths.length === 0) {
return false
}
const unstagedPaths = new Set(grouped.unstaged.map((entry) => entry.path))
const unstagedPaths = new Set(stageableUnstagedPaths)
return grouped.staged.some((entry) => unstagedPaths.has(entry.path))
}, [grouped.staged, grouped.unstaged])
}, [grouped.staged, stageableUnstagedPaths])
const primaryAction: PrimaryAction = useMemo(() => {
const action = resolvePrimaryAction({
stagedCount: grouped.staged.length,
hasUnstagedChanges,
hasStageableChanges,
hasPartiallyStagedChanges,
hasMessage: commitMessage.trim().length > 0,
hasUnresolvedConflicts: unresolvedConflicts.length > 0,
@ -2884,6 +2897,7 @@ function SourceControlInner(): React.JSX.Element {
commitMessage,
grouped.staged.length,
hasUnstagedChanges,
hasStageableChanges,
hasPartiallyStagedChanges,
isCommitting,
isAbortingOperation,
@ -2905,6 +2919,7 @@ function SourceControlInner(): React.JSX.Element {
resolveDropdownItems({
stagedCount: grouped.staged.length,
hasUnstagedChanges,
hasStageableChanges,
hasPartiallyStagedChanges,
hasMessage: commitMessage.trim().length > 0,
hasUnresolvedConflicts: unresolvedConflicts.length > 0,
@ -2925,6 +2940,7 @@ function SourceControlInner(): React.JSX.Element {
commitMessage,
grouped.staged.length,
hasUnstagedChanges,
hasStageableChanges,
hasPartiallyStagedChanges,
isCommitting,
conflictOperation,
@ -3106,11 +3122,7 @@ function SourceControlInner(): React.JSX.Element {
const bulkStagePaths = useMemo(
() =>
selectedEntries
.filter(
(entry) =>
(entry.area === 'unstaged' || entry.area === 'untracked') &&
entry.entry.conflictStatus !== 'unresolved'
)
.filter((entry) => isStageableStatusEntry(entry.entry))
.map((entry) => entry.entry.path),
[selectedEntries]
)
@ -6590,6 +6602,7 @@ const UncommittedEntryRow = React.memo(function UncommittedEntryRow({
const dirPath = parentDir === '.' ? '' : parentDir
const isUnresolvedConflict = entry.conflictStatus === 'unresolved'
const isResolvedLocally = entry.conflictStatus === 'resolved_locally'
const isSubmoduleWorktreeOnly = isSubmoduleWorktreeOnlyChange(entry)
const conflictLabel = entry.conflictKind ? CONFLICT_KIND_LABELS[entry.conflictKind] : null
// Why: the hint text ("Open and edit…", "Decide whether to…") was removed
// from the sidebar because it's not actionable here — the user can only
@ -6607,8 +6620,7 @@ const UncommittedEntryRow = React.memo(function UncommittedEntryRow({
!isUnresolvedConflict &&
!isResolvedLocally &&
(entry.area === 'unstaged' || entry.area === 'untracked')
const canStage =
!isUnresolvedConflict && (entry.area === 'unstaged' || entry.area === 'untracked')
const canStage = isStageableStatusEntry(entry)
const canUnstage = entry.area === 'staged'
return (
@ -6662,8 +6674,10 @@ const UncommittedEntryRow = React.memo(function UncommittedEntryRow({
<span className="ml-1.5 text-[11px] text-muted-foreground">{dirPath}</span>
)}
</span>
{conflictLabel && (
<div className="truncate text-[11px] text-muted-foreground">{conflictLabel}</div>
{(conflictLabel || isSubmoduleWorktreeOnly) && (
<div className="truncate text-[11px] text-muted-foreground">
{conflictLabel ?? SUBMODULE_WORKTREE_ONLY_LABEL}
</div>
)}
</div>
{commentCount > 0 && (
@ -6708,14 +6722,15 @@ const UncommittedEntryRow = React.memo(function UncommittedEntryRow({
}}
/>
)}
{canStage && (
{(canStage || isSubmoduleWorktreeOnly) && (
<ActionButton
icon={Plus}
title="Stage"
title={isSubmoduleWorktreeOnly ? SUBMODULE_WORKTREE_ONLY_STAGE_TOOLTIP : 'Stage'}
onClick={(event) => {
event.stopPropagation()
void onStage(entry.path)
}}
disabled={isSubmoduleWorktreeOnly}
/>
)}
{canUnstage && (

42
src/renderer/src/components/right-sidebar/discard-all-sequence.test.ts
View File

@ -3,6 +3,8 @@ import {
getDiscardAllPaths,
getStageAllPaths,
getUnstageAllPaths,
isStageableStatusEntry,
isSubmoduleWorktreeOnlyChange,
runDiscardAllForArea,
type DiscardAllArea
} from './discard-all-sequence'
@ -109,12 +111,52 @@ describe('getStageAllPaths', () => {
expect(getStageAllPaths(entries, 'unstaged')).toEqual(['clean.ts', 'resolved.ts'])
})
it('skips submodule rows that only contain nested worktree dirtiness', () => {
const entries: GitStatusEntry[] = [
entry({
path: 'nested-repo',
area: 'unstaged',
submodule: { commitChanged: false, trackedChanges: false, untrackedChanges: true }
}),
entry({
path: 'changed-gitlink',
area: 'unstaged',
submodule: { commitChanged: true, trackedChanges: false, untrackedChanges: true }
})
]
expect(getStageAllPaths(entries, 'unstaged')).toEqual(['changed-gitlink'])
})
it('returns an empty array when nothing matches', () => {
expect(getStageAllPaths([], 'unstaged')).toEqual([])
expect(getStageAllPaths([entry({ path: 'a.ts', area: 'staged' })], 'unstaged')).toEqual([])
})
})
describe('status entry stageability', () => {
it('marks nested-only submodule changes as not stageable from the parent repo', () => {
const nestedOnly = entry({
path: 'nested-repo',
area: 'unstaged',
submodule: { commitChanged: false, trackedChanges: true, untrackedChanges: false }
})
expect(isSubmoduleWorktreeOnlyChange(nestedOnly)).toBe(true)
expect(isStageableStatusEntry(nestedOnly)).toBe(false)
})
it('keeps changed submodule gitlinks stageable from the parent repo', () => {
const changedGitlink = entry({
path: 'nested-repo',
area: 'unstaged',
submodule: { commitChanged: true, trackedChanges: true, untrackedChanges: true }
})
expect(isSubmoduleWorktreeOnlyChange(changedGitlink)).toBe(false)
expect(isStageableStatusEntry(changedGitlink)).toBe(true)
})
})
describe('getUnstageAllPaths', () => {
it('returns only staged-area paths', () => {
const entries: GitStatusEntry[] = [

17
src/renderer/src/components/right-sidebar/discard-all-sequence.ts
View File

@ -32,10 +32,25 @@ export type StageAllArea = 'unstaged' | 'untracked'
*/
export function getStageAllPaths(entries: readonly GitStatusEntry[], area: StageAllArea): string[] {
return entries
.filter((entry) => entry.area === area && entry.conflictStatus !== 'unresolved')
.filter((entry) => entry.area === area && isStageableStatusEntry(entry))
.map((entry) => entry.path)
}
export function isStageableStatusEntry(entry: GitStatusEntry): boolean {
return (
(entry.area === 'unstaged' || entry.area === 'untracked') &&
entry.conflictStatus !== 'unresolved' &&
!isSubmoduleWorktreeOnlyChange(entry)
)
}
export function isSubmoduleWorktreeOnlyChange(entry: GitStatusEntry): boolean {
const submodule = entry.submodule
// Why: parent-repo `git add <submodule>` can stage a changed gitlink commit,
// but it cannot stage tracked/untracked file dirtiness inside the submodule.
return entry.area === 'unstaged' && !!submodule && !submodule.commitChanged
}
/**
* Collect the paths an "Unstage all" action should operate on.
* Every staged row is eligible — `git reset HEAD` on a staged conflict

1
src/renderer/src/components/right-sidebar/source-control-dropdown-items.test.ts
View File

@ -8,6 +8,7 @@ function inputs(overrides: Partial<DropdownActionInputs> = {}): DropdownActionIn
return {
stagedCount: 0,
hasUnstagedChanges: false,
hasStageableChanges: false,
hasPartiallyStagedChanges: false,
hasMessage: false,
hasUnresolvedConflicts: false,

29
src/renderer/src/components/right-sidebar/source-control-primary-action.test.ts
View File

@ -8,6 +8,7 @@ function inputs(overrides: Partial<PrimaryActionInputs> = {}): PrimaryActionInpu
return {
stagedCount: 0,
hasUnstagedChanges: false,
hasStageableChanges: false,
hasPartiallyStagedChanges: false,
hasMessage: false,
hasUnresolvedConflicts: false,
@ -287,6 +288,7 @@ describe('resolvePrimaryAction', () => {
const result = resolvePrimaryAction(
inputs({
hasUnstagedChanges: true,
hasStageableChanges: true,
upstreamStatus: { hasUpstream: true, ahead: 0, behind: 3 }
})
)
@ -302,6 +304,7 @@ describe('resolvePrimaryAction', () => {
const result = resolvePrimaryAction(
inputs({
hasUnstagedChanges: true,
hasStageableChanges: true,
upstreamStatus: { hasUpstream: true, ahead: 2, behind: 0 }
})
)
@ -314,6 +317,7 @@ describe('resolvePrimaryAction', () => {
const result = resolvePrimaryAction(
inputs({
hasUnstagedChanges: true,
hasStageableChanges: true,
upstreamStatus: { hasUpstream: false, ahead: 0, behind: 0 }
})
)
@ -322,7 +326,7 @@ describe('resolvePrimaryAction', () => {
it('returns Stage All on a dirty tree while upstream status is still loading', () => {
const result = resolvePrimaryAction(
inputs({ hasUnstagedChanges: true, upstreamStatus: undefined })
inputs({ hasUnstagedChanges: true, hasStageableChanges: true, upstreamStatus: undefined })
)
expect(result.kind).toBe('stage')
expect(result.disabled).toBe(false)
@ -333,6 +337,7 @@ describe('resolvePrimaryAction', () => {
inputs({
stagedCount: 1,
hasUnstagedChanges: true,
hasStageableChanges: true,
hasPartiallyStagedChanges: true,
hasMessage: true,
upstreamStatus: { hasUpstream: true, ahead: 0, behind: 0 }
@ -357,6 +362,22 @@ describe('resolvePrimaryAction', () => {
expect(result.disabled).toBe(false)
})
it('does not return Stage All when dirty rows cannot be staged from the parent repo', () => {
const result = resolvePrimaryAction(
inputs({
hasUnstagedChanges: true,
hasStageableChanges: false,
upstreamStatus: upstreamInSync
})
)
expect(result).toEqual({
kind: 'commit',
label: 'Commit',
title: 'Stage at least one file to commit',
disabled: true
})
})
it('still disables Commit (needs message) when staged+dirty without a message', () => {
const result = resolvePrimaryAction(
inputs({ stagedCount: 1, hasUnstagedChanges: true, hasMessage: false })
@ -368,7 +389,11 @@ describe('resolvePrimaryAction', () => {
it('returns Stage All when unstaged changes exist on an in-sync branch', () => {
const result = resolvePrimaryAction(
inputs({ hasUnstagedChanges: true, upstreamStatus: upstreamInSync })
inputs({
hasUnstagedChanges: true,
hasStageableChanges: true,
upstreamStatus: upstreamInSync
})
)
expect(result).toEqual({
kind: 'stage',

4
src/renderer/src/components/right-sidebar/source-control-primary-action.ts
View File

@ -50,6 +50,7 @@ export type PrimaryAction = {
export type PrimaryActionInputs = {
stagedCount: number
hasUnstagedChanges: boolean
hasStageableChanges: boolean
hasPartiallyStagedChanges: boolean
hasMessage: boolean
hasUnresolvedConflicts: boolean
@ -130,6 +131,7 @@ export function resolvePrimaryAction(inputs: PrimaryActionInputs): PrimaryAction
const {
stagedCount,
hasUnstagedChanges,
hasStageableChanges,
hasPartiallyStagedChanges,
hasMessage,
hasUnresolvedConflicts,
@ -253,7 +255,7 @@ export function resolvePrimaryAction(inputs: PrimaryActionInputs): PrimaryAction
// with uncommitted changes; push/publish skips the actual user need).
// Sits before the upstream-status checks so it works regardless of
// whether upstream has resolved yet.
if (!hasStaged && hasUnstagedChanges) {
if (!hasStaged && hasStageableChanges) {
return {
kind: 'stage',
label: 'Stage All',

6
src/shared/git-status-types.ts
View File

@ -12,6 +12,11 @@ export type GitConflictKind =
export type GitConflictResolutionStatus = 'unresolved' | 'resolved_locally'
export type GitConflictStatusSource = 'git' | 'session'
export type GitConflictOperation = 'merge' | 'rebase' | 'cherry-pick' | 'unknown'
export type GitSubmoduleStatus = {
commitChanged: boolean
trackedChanges: boolean
untrackedChanges: boolean
}
// Compatibility note for non-upgraded consumers:
// Any consumer that has not been upgraded to read `conflictStatus` may still
@ -32,6 +37,7 @@ export type GitUncommittedEntry = {
conflictKind?: GitConflictKind
conflictStatus?: GitConflictResolutionStatus
conflictStatusSource?: GitConflictStatusSource
submodule?: GitSubmoduleStatus
// Working-tree line counts for this entry's staging area (staged vs unstaged
// diffs are reported separately). Untracked files count their full contents
// as additions. Undefined for binary files and when the diff is unavailable.

1
src/shared/types.ts
View File

@ -39,6 +39,7 @@ export type {
GitStagingArea,
GitStatusEntry,
GitStatusResult,
GitSubmoduleStatus,
GitUncommittedEntry,
GitUpstreamStatus
} from './git-status-types'