From 86a24410a4972f7e47861ec30fcbfbd0ff99456a Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 14 Jun 2026 00:20:01 -0700 Subject: [PATCH] Fix source control row preview opens and environment discard scoping (#5346) - Support opening uncommitted and branch diffs as preview editors on click, and permanently on double-click - Pass active runtime environment ID to editor autosave quiesce and file change notification on discard - Prevent staging submodule worktree-only changes from the parent repo, showing clear guidance labels instead --- .../SourceControl.preview-open.test.tsx | 466 ++++++++++++++++++ .../right-sidebar/SourceControl.tsx | 67 ++- 2 files changed, 512 insertions(+), 21 deletions(-) create mode 100644 src/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsx diff --git a/src/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsx b/src/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsx new file mode 100644 index 00000000000..857d043174d --- /dev/null +++ b/src/renderer/src/components/right-sidebar/SourceControl.preview-open.test.tsx @@ -0,0 +1,466 @@ +// @vitest-environment happy-dom + +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { TooltipProvider } from '@/components/ui/tooltip' +import type { + GitBranchChangeEntry, + GitBranchCompareSummary, + GitStatusEntry +} from '../../../../shared/types' +import SourceControl from './SourceControl' + +const mocks = vi.hoisted(() => { + const activeRepo = { + id: 'repo-1', + path: '/repo', + displayName: 'Repo', + badgeColor: '#000', + addedAt: 0 + } + const activeWorktree = { + id: 'wt-1', + repoId: 'repo-1', + path: '/repo/wt', + head: 'abcdef123', + branch: 'refs/heads/feature/source-control-preview', + isBare: false, + isMainWorktree: false, + displayName: 'feature/source-control-preview', + comment: '', + linkedIssue: null, + linkedPR: null, + linkedLinearIssue: null, + linkedGitLabMR: null, + linkedGitLabIssue: null, + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 0, + lastActivityAt: 0 + } + const calls = { + openDiff: vi.fn(), + openFile: vi.fn(), + openConflictFile: vi.fn(), + openBranchDiff: vi.fn(), + createEmptySplitGroup: vi.fn(), + discardRuntimeGitPath: vi.fn(), + refreshGitStatusForWorktree: vi.fn(), + requestEditorSaveQuiesce: vi.fn(), + notifyEditorExternalFileChange: vi.fn() + } + return { + activeRepo, + activeWorktree, + calls, + state: {} as Record + } +}) + +vi.mock('@/store', () => { + const useAppStore = Object.assign( + (selector?: (state: Record) => unknown) => + selector ? selector(mocks.state) : mocks.state, + { + getState: () => mocks.state + } + ) + return { useAppStore } +}) + +vi.mock('@/store/selectors', () => ({ + useActiveWorktree: () => mocks.activeWorktree, + useRepoById: (repoId: string | null) => + repoId === mocks.activeRepo.id ? mocks.activeRepo : null, + useWorktreeMap: () => new Map([[mocks.activeWorktree.id, mocks.activeWorktree]]) +})) + +vi.mock('@/components/confirmation-dialog', () => ({ + useConfirmationDialog: () => vi.fn().mockResolvedValue(true) +})) + +vi.mock('@/runtime/runtime-git-client', async (importOriginal) => { + const actual = await importOriginal>() + return { + ...actual, + discardRuntimeGitPath: mocks.calls.discardRuntimeGitPath + } +}) + +vi.mock('@/components/editor/editor-autosave', () => ({ + requestEditorSaveQuiesce: mocks.calls.requestEditorSaveQuiesce, + notifyEditorExternalFileChange: mocks.calls.notifyEditorExternalFileChange +})) + +vi.mock('./git-status-refresh', () => ({ + refreshGitStatusForWorktree: mocks.calls.refreshGitStatusForWorktree +})) + +function gitEntry(overrides: Partial): GitStatusEntry { + return { + path: 'src/file.ts', + area: 'unstaged', + status: 'modified', + added: 1, + removed: 0, + ...overrides + } +} + +function branchEntry(overrides: Partial = {}): GitBranchChangeEntry { + return { + path: 'src/branch.ts', + status: 'modified', + added: 2, + removed: 1, + ...overrides + } +} + +function branchSummary(): GitBranchCompareSummary { + return { + baseRef: 'origin/main', + baseOid: 'base', + compareRef: 'feature/source-control-preview', + headOid: 'head', + mergeBase: 'base', + changedFiles: 1, + commitsAhead: 1, + status: 'ready' + } +} + +function noopAsync(value: unknown = undefined): () => Promise { + return vi.fn().mockResolvedValue(value) +} + +function resetState(overrides: Partial> = {}): void { + vi.clearAllMocks() + mocks.calls.createEmptySplitGroup.mockReturnValue('group-2') + mocks.calls.discardRuntimeGitPath.mockResolvedValue(undefined) + mocks.calls.refreshGitStatusForWorktree.mockResolvedValue(undefined) + mocks.calls.requestEditorSaveQuiesce.mockResolvedValue(undefined) + mocks.state = { + activeWorktreeId: mocks.activeWorktree.id, + activeGroupIdByWorktree: { [mocks.activeWorktree.id]: 'group-1' }, + groupsByWorktree: { [mocks.activeWorktree.id]: [{ id: 'group-1', activeTabId: null }] }, + repos: [mocks.activeRepo], + worktreesByRepo: { [mocks.activeRepo.id]: [mocks.activeWorktree] }, + rightSidebarOpen: false, + rightSidebarTab: 'source-control', + gitStatusByWorktree: { [mocks.activeWorktree.id]: [] }, + gitBranchChangesByWorktree: { [mocks.activeWorktree.id]: [] }, + gitBranchCompareSummaryByWorktree: { [mocks.activeWorktree.id]: null }, + gitConflictOperationByWorktree: {}, + remoteStatusesByWorktree: {}, + isRemoteOperationActive: false, + inFlightRemoteOpKind: null, + settings: null, + hostedReviewCache: {}, + prCache: {}, + commitMessageGenerationRecords: {}, + pullRequestGenerationRecords: {}, + getDiffComments: vi.fn(() => []), + updateSettings: noopAsync(), + openSettingsTarget: vi.fn(), + openSettingsPage: vi.fn(), + fetchHostedReviewForBranch: noopAsync(), + getHostedReviewCreationEligibility: noopAsync(null), + createHostedReview: noopAsync({ ok: false, error: 'not available' }), + updateWorktreeMeta: noopAsync(), + fetchPRForBranch: noopAsync(), + enqueueGitHubPRRefresh: vi.fn(), + updateRepo: noopAsync(), + setGitStatus: vi.fn(), + updateWorktreeGitIdentity: vi.fn(), + beginGitBranchCompareRequest: vi.fn(() => 'request-key'), + setGitBranchCompareResult: vi.fn(), + fetchUpstreamStatus: noopAsync(), + setUpstreamStatus: vi.fn(), + pushBranch: noopAsync(), + pullBranch: noopAsync(), + fastForwardBranch: noopAsync(), + syncBranch: noopAsync(), + rebaseFromBase: noopAsync(), + fetchBranch: noopAsync(), + revealInExplorer: vi.fn(), + trackConflictPath: vi.fn(), + openDiff: mocks.calls.openDiff, + openFile: mocks.calls.openFile, + setEditorViewMode: vi.fn(), + setMarkdownViewMode: vi.fn(), + setPendingEditorReveal: vi.fn(), + openConflictFile: mocks.calls.openConflictFile, + openConflictReview: vi.fn(), + openBranchDiff: mocks.calls.openBranchDiff, + createEmptySplitGroup: mocks.calls.createEmptySplitGroup, + openAllDiffs: vi.fn(), + openBranchAllDiffs: vi.fn(), + openCommitAllDiffs: vi.fn(), + deleteDiffComment: noopAsync(true), + clearDiffComments: noopAsync(true), + clearDiffCommentsForFile: noopAsync(true), + setScrollToDiffCommentId: vi.fn(), + setRightSidebarOpen: vi.fn(), + setRightSidebarTab: vi.fn(), + allocateCommitMessageGenerationRequestId: vi.fn(() => 'commit-generation-1'), + setCommitMessageGenerationRecord: vi.fn(), + updateCommitMessageGenerationRecord: vi.fn(), + pruneCommitMessageGenerationRecords: vi.fn(), + allocatePullRequestGenerationRequestId: vi.fn(() => 'pr-generation-1'), + setPullRequestGenerationRecord: vi.fn(), + updatePullRequestGenerationRecord: vi.fn(), + prunePullRequestGenerationRecords: vi.fn(), + ...overrides + } +} + +let container: HTMLDivElement +let root: Root + +beforeEach(() => { + globalThis.IS_REACT_ACT_ENVIRONMENT = true + resetState() + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) +}) + +afterEach(() => { + act(() => root.unmount()) + container.remove() +}) + +function renderSourceControl(): void { + act(() => { + root.render( + + + + ) + }) +} + +function clickUncommitted(path: string, init: MouseEventInit = {}): void { + const row = container.querySelector(`[data-source-control-path="${path}"]`) + expect(row).not.toBeNull() + act(() => { + row?.dispatchEvent(new MouseEvent('click', { bubbles: true, ...init })) + }) +} + +function doubleClickUncommitted(path: string): void { + const row = container.querySelector(`[data-source-control-path="${path}"]`) + expect(row).not.toBeNull() + act(() => { + row?.dispatchEvent(new MouseEvent('dblclick', { bubbles: true })) + }) +} + +function clickBranchRow(init: MouseEventInit = {}): void { + const label = [...container.querySelectorAll('span')].find( + (candidate) => candidate.textContent === 'branch.ts' + ) + const row = label?.closest('div') + expect(row).not.toBeNull() + act(() => { + row?.dispatchEvent(new MouseEvent('click', { bubbles: true, ...init })) + }) +} + +describe('SourceControl preview row opens', () => { + it('passes preview=true when plain uncommitted row clicks open diff tabs', () => { + resetState({ + gitStatusByWorktree: { + [mocks.activeWorktree.id]: [ + gitEntry({ path: 'src/file.ts' }), + gitEntry({ path: 'src/staged.ts', area: 'staged' }) + ] + } + }) + renderSourceControl() + + clickUncommitted('src/file.ts') + clickUncommitted('src/staged.ts') + + expect(mocks.calls.openDiff).toHaveBeenCalledWith( + mocks.activeWorktree.id, + '/repo/wt/src/file.ts', + 'src/file.ts', + 'typescript', + false, + { targetGroupId: undefined, preview: true } + ) + expect(mocks.calls.openDiff).toHaveBeenCalledWith( + mocks.activeWorktree.id, + '/repo/wt/src/staged.ts', + 'src/staged.ts', + 'typescript', + true, + { targetGroupId: undefined, preview: true } + ) + }) + + it('keeps modifier split row opens permanent and targeted at the split group', () => { + resetState({ + gitStatusByWorktree: { [mocks.activeWorktree.id]: [gitEntry({ path: 'src/file.ts' })] } + }) + renderSourceControl() + + clickUncommitted('src/file.ts', { ctrlKey: true }) + + expect(mocks.calls.createEmptySplitGroup).toHaveBeenCalledWith( + mocks.activeWorktree.id, + 'group-1', + 'right' + ) + expect(mocks.calls.openDiff).toHaveBeenCalledWith( + mocks.activeWorktree.id, + '/repo/wt/src/file.ts', + 'src/file.ts', + 'typescript', + false, + { targetGroupId: 'group-2', preview: false } + ) + }) + + it('keeps explicit permanent uncommitted opens permanent', () => { + resetState({ + gitStatusByWorktree: { [mocks.activeWorktree.id]: [gitEntry({ path: 'src/file.ts' })] } + }) + renderSourceControl() + + doubleClickUncommitted('src/file.ts') + + expect(mocks.calls.openDiff).toHaveBeenCalledWith( + mocks.activeWorktree.id, + '/repo/wt/src/file.ts', + 'src/file.ts', + 'typescript', + false, + { targetGroupId: undefined, preview: false } + ) + }) + + it('passes preview through markdown edit-in-changes and conflict file opens', () => { + resetState({ + gitStatusByWorktree: { + [mocks.activeWorktree.id]: [ + gitEntry({ path: 'docs/readme.md' }), + gitEntry({ + path: 'src/conflict.ts', + conflictKind: 'both_modified', + conflictStatus: 'unresolved' + }) + ] + } + }) + renderSourceControl() + + clickUncommitted('docs/readme.md') + clickUncommitted('src/conflict.ts') + + expect(mocks.calls.openFile).toHaveBeenCalledWith( + { + filePath: '/repo/wt/docs/readme.md', + relativePath: 'docs/readme.md', + worktreeId: mocks.activeWorktree.id, + language: 'markdown', + mode: 'edit' + }, + { targetGroupId: undefined, preview: true } + ) + expect(mocks.calls.openConflictFile).toHaveBeenCalledWith( + mocks.activeWorktree.id, + '/repo/wt', + expect.objectContaining({ path: 'src/conflict.ts' }), + 'typescript', + { targetGroupId: undefined, preview: true } + ) + }) + + it('scopes discard autosave quiesce and reload notifications to the active runtime', async () => { + resetState({ + gitStatusByWorktree: { [mocks.activeWorktree.id]: [gitEntry({ path: 'src/file.ts' })] } + }) + renderSourceControl() + mocks.state.settings = { activeRuntimeEnvironmentId: 'runtime-remote' } + + const row = container.querySelector('[data-source-control-path="src/file.ts"]') + const discardButton = row?.querySelector( + 'button[aria-label="Discard changes"]' + ) + expect(discardButton).not.toBeNull() + act(() => { + discardButton?.dispatchEvent(new MouseEvent('click', { bubbles: true })) + }) + + const confirmButton = [...document.body.querySelectorAll('button')].find( + (button) => button.textContent?.trim() === 'Discard' + ) + expect(confirmButton).not.toBeNull() + await act(async () => { + confirmButton?.dispatchEvent(new MouseEvent('click', { bubbles: true })) + await Promise.resolve() + }) + + expect(mocks.calls.requestEditorSaveQuiesce).toHaveBeenCalledWith({ + worktreeId: mocks.activeWorktree.id, + worktreePath: '/repo/wt', + relativePath: 'src/file.ts', + runtimeEnvironmentId: 'runtime-remote' + }) + expect(mocks.calls.notifyEditorExternalFileChange).toHaveBeenCalledWith({ + worktreeId: mocks.activeWorktree.id, + worktreePath: '/repo/wt', + relativePath: 'src/file.ts', + runtimeEnvironmentId: 'runtime-remote' + }) + }) + + it('keeps nested-only submodule rows non-stageable from the parent repo', () => { + resetState({ + gitStatusByWorktree: { + [mocks.activeWorktree.id]: [ + gitEntry({ + path: 'packages/nested', + submodule: { commitChanged: false, trackedChanges: true, untrackedChanges: false } + }) + ] + } + }) + renderSourceControl() + + const row = container.querySelector( + '[data-source-control-path="packages/nested"]' + ) + expect(row?.textContent).toContain('Submodule changes - stage inside submodule') + + const stageButton = row?.querySelector( + 'button[aria-label="Stage these changes inside the submodule"]' + ) + expect(stageButton).not.toBeNull() + expect(stageButton?.getAttribute('aria-disabled')).toBe('true') + }) + + it('passes preview=true when a plain branch row click opens a branch diff tab', () => { + resetState({ + gitBranchChangesByWorktree: { [mocks.activeWorktree.id]: [branchEntry()] }, + gitBranchCompareSummaryByWorktree: { [mocks.activeWorktree.id]: branchSummary() } + }) + renderSourceControl() + + clickBranchRow() + + expect(mocks.calls.openBranchDiff).toHaveBeenCalledWith( + mocks.activeWorktree.id, + '/repo/wt', + expect.objectContaining({ path: 'src/branch.ts' }), + expect.objectContaining({ status: 'ready' }), + 'typescript', + { targetGroupId: undefined, preview: true } + ) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index 18a3c9cdece..db24f343971 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -67,6 +67,8 @@ import { getDiscardAllPaths, getStageAllPaths, getUnstageAllPaths, + isStageableStatusEntry, + isSubmoduleWorktreeOnlyChange, runDiscardAllForArea, type DiscardAllArea } from './discard-all-sequence' @@ -182,6 +184,8 @@ import { import { hasExpandedCommitFailureDetails, summarizeCommitFailure } from './commit-failure-summary' import { isSourceControlSplitOpenModifier, + shouldOpenSourceControlRowAsPreview, + toPermanentSourceControlRowOpenEvent, type SourceControlRowOpenEvent } from './source-control-split-open' import { SourceControlAgentActionDialog } from './SourceControlAgentActionDialog' @@ -296,6 +300,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 { return new Set(DEFAULT_COLLAPSED_SECTIONS) @@ -2582,12 +2588,14 @@ function SourceControlInner(): React.JSX.Element { return } const targetGroupId = resolveSplitTargetGroupId(event) + const openAsPreview = shouldOpenSourceControlRowAsPreview(event, targetGroupId) if (entry.conflictKind && entry.conflictStatus) { if (entry.conflictStatus === 'unresolved') { trackConflictPath(activeWorktreeId, entry.path, entry.conflictKind) } openConflictFile(activeWorktreeId, worktreePath, entry, detectLanguage(entry.path), { - targetGroupId + targetGroupId, + preview: openAsPreview }) return } @@ -2610,13 +2618,14 @@ function SourceControlInner(): React.JSX.Element { language, mode: 'edit' }, - { targetGroupId } + { targetGroupId, preview: openAsPreview } ) setEditorViewMode(filePath, 'changes') return } openDiff(activeWorktreeId, filePath, entry.path, language, entry.area === 'staged', { - targetGroupId + targetGroupId, + preview: openAsPreview }) }, [ @@ -2665,11 +2674,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] ) @@ -3225,13 +3230,14 @@ function SourceControlInner(): React.JSX.Element { ) { return } + const targetGroupId = resolveSplitTargetGroupId(event) openBranchDiff( activeWorktreeId, worktreePath, entry, branchSummary, detectLanguage(entry.path), - { targetGroupId: resolveSplitTargetGroupId(event) } + { targetGroupId, preview: shouldOpenSourceControlRowAsPreview(event, targetGroupId) } ) }, [activeWorktreeId, branchSummary, openBranchDiff, resolveSplitTargetGroupId, worktreePath] @@ -3451,13 +3457,16 @@ function SourceControlInner(): React.JSX.Element { if (!worktreePath || !activeWorktreeId) { return } + const runtimeEnvironmentId = + useAppStore.getState().settings?.activeRuntimeEnvironmentId?.trim() || null // Why: git discard replaces the working tree version of this file. Any // pending editor autosave must be quiesced first so it cannot recreate // the discarded edits after git restores the file. await requestEditorSaveQuiesce({ worktreeId: activeWorktreeId, worktreePath, - relativePath: filePath + relativePath: filePath, + runtimeEnvironmentId }) const connectionId = getConnectionId(activeWorktreeId ?? null) ?? undefined await discardRuntimeGitPath( @@ -3473,7 +3482,8 @@ function SourceControlInner(): React.JSX.Element { notifyEditorExternalFileChange({ worktreeId: activeWorktreeId, worktreePath, - relativePath: filePath + relativePath: filePath, + runtimeEnvironmentId }) }, [activeRepoSettings, activeWorktreeId, worktreePath] @@ -3484,6 +3494,8 @@ function SourceControlInner(): React.JSX.Element { if (!worktreePath || !activeWorktreeId) { return } + const runtimeEnvironmentId = + useAppStore.getState().settings?.activeRuntimeEnvironmentId?.trim() || null // Why: bulk discard replaces many working-tree files at once. Quiesce // any matching editor autosaves before git mutates the files so a delayed // save cannot recreate edits after the restore. @@ -3492,7 +3504,8 @@ function SourceControlInner(): React.JSX.Element { requestEditorSaveQuiesce({ worktreeId: activeWorktreeId, worktreePath, - relativePath + relativePath, + runtimeEnvironmentId }) ) ) @@ -3511,7 +3524,8 @@ function SourceControlInner(): React.JSX.Element { notifyEditorExternalFileChange({ worktreeId: activeWorktreeId, worktreePath, - relativePath + relativePath, + runtimeEnvironmentId }) } }, @@ -6121,6 +6135,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 ? getLocalizedConflictKindLabel(entry.conflictKind) : null @@ -6140,8 +6155,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 ( @@ -6183,6 +6197,9 @@ const UncommittedEntryRow = React.memo(function UncommittedEntryRow({ onOpen(entry, e) } }} + onDoubleClick={(e) => { + onOpen(entry, toPermanentSourceControlRowOpenEvent(e)) + }} >
@@ -6192,8 +6209,10 @@ const UncommittedEntryRow = React.memo(function UncommittedEntryRow({ {dirPath} )} - {conflictLabel && ( -
{conflictLabel}
+ {(conflictLabel || isSubmoduleWorktreeOnly) && ( +
+ {conflictLabel ?? SUBMODULE_WORKTREE_ONLY_LABEL} +
)}
{commentCount > 0 && ( @@ -6251,14 +6270,19 @@ const UncommittedEntryRow = React.memo(function UncommittedEntryRow({ }} /> )} - {canStage && ( + {(canStage || isSubmoduleWorktreeOnly) && ( { event.stopPropagation() void onStage(entry.path) }} + disabled={isSubmoduleWorktreeOnly} /> )} {canUnstage && ( @@ -6348,7 +6372,7 @@ function BranchEntryRow({ worktreePath: string depth?: number onRevealInExplorer: (worktreeId: string, absolutePath: string) => void - onOpen: (event: React.MouseEvent) => void + onOpen: (event: SourceControlRowOpenEvent) => void commentCount: number showPathHint?: boolean }): React.JSX.Element { @@ -6374,7 +6398,8 @@ function BranchEntryRow({ e.dataTransfer.setData(WORKSPACE_FILE_PATH_MIME, absolutePath) e.dataTransfer.effectAllowed = 'copy' }} - onClick={onOpen} + onClick={(e) => onOpen(e)} + onDoubleClick={(e) => onOpen(toPermanentSourceControlRowOpenEvent(e))} >