Prevent auto-reloading diff views when status area shifts (#5951)
* Avoid auto-reloading single-file diff tabs if their status row has moved to a different area (e.g. from unstaged to staged). * Prevent auto-reloading combined uncommitted diffs when they are backed by an uncommitted entries snapshot. * This preserves the open tab's original diff context as staging and commit status changes, preventing useful context from being lost.
This commit is contained in:
parent
308da355bc
commit
954fa0a65e
|
|
@ -193,8 +193,7 @@ export function buildHeadlessTabGroupSplit(args: {
|
|||
return {
|
||||
...group,
|
||||
tabOrder: sourceOrder,
|
||||
activeTabId:
|
||||
group.activeTabId === args.tabId ? (sourceOrder[0] ?? null) : group.activeTabId
|
||||
activeTabId: group.activeTabId === args.tabId ? (sourceOrder[0] ?? null) : group.activeTabId
|
||||
}
|
||||
}
|
||||
return group
|
||||
|
|
|
|||
|
|
@ -55,7 +55,11 @@ import {
|
|||
ORCA_EDITOR_EXTERNAL_FILE_CHANGE_EVENT,
|
||||
type EditorPathMutationTarget
|
||||
} from './editor-autosave'
|
||||
import { getCombinedBranchEntries, getCombinedUncommittedEntries } from './combined-diff-entries'
|
||||
import {
|
||||
getCombinedBranchEntries,
|
||||
getCombinedUncommittedEntries,
|
||||
shouldAutoReloadCombinedDiffFromGitStatus
|
||||
} from './combined-diff-entries'
|
||||
import { getCombinedDiffCommitMessageBody } from './combined-diff-commit-message'
|
||||
import { getDiffSectionEstimatedHeight, isIntrinsicHeightImageDiff } from './diff-section-layout'
|
||||
import { getLargeDiffRenderLimit } from './large-diff-render-limit'
|
||||
|
|
@ -387,6 +391,11 @@ export default function CombinedDiffViewer({
|
|||
)
|
||||
const entries = isBranchMode ? branchEntries : isCommitMode ? commitEntries : uncommittedEntries
|
||||
const treeMode = isBranchMode ? 'branch' : isCommitMode ? 'commit' : 'uncommitted'
|
||||
const hasUncommittedEntriesSnapshot = file.uncommittedEntriesSnapshot !== undefined
|
||||
const shouldAutoReloadFromGitStatus = shouldAutoReloadCombinedDiffFromGitStatus({
|
||||
mode: treeMode,
|
||||
hasUncommittedEntriesSnapshot
|
||||
})
|
||||
const entrySignature = React.useMemo(
|
||||
() =>
|
||||
JSON.stringify({
|
||||
|
|
@ -820,15 +829,15 @@ export default function CombinedDiffViewer({
|
|||
)
|
||||
|
||||
const combinedGitStatusSignature = React.useMemo(() => {
|
||||
if (treeMode !== 'uncommitted') {
|
||||
if (!shouldAutoReloadFromGitStatus) {
|
||||
return ''
|
||||
}
|
||||
return buildCombinedGitStatusSignature(sections, gitStatusEntries)
|
||||
}, [gitStatusEntries, sections, treeMode])
|
||||
}, [gitStatusEntries, sections, shouldAutoReloadFromGitStatus])
|
||||
const prevCombinedGitStatusSignatureRef = useRef<string | null>(null)
|
||||
|
||||
useEffect(() => {
|
||||
if (treeMode !== 'uncommitted') {
|
||||
if (!shouldAutoReloadFromGitStatus) {
|
||||
prevCombinedGitStatusSignatureRef.current = null
|
||||
return
|
||||
}
|
||||
|
|
@ -843,7 +852,7 @@ export default function CombinedDiffViewer({
|
|||
for (const index of loadedIndicesRef.current) {
|
||||
requestCombinedDiffSectionReload(index)
|
||||
}
|
||||
}, [combinedGitStatusSignature, requestCombinedDiffSectionReload, treeMode])
|
||||
}, [combinedGitStatusSignature, requestCombinedDiffSectionReload, shouldAutoReloadFromGitStatus])
|
||||
|
||||
useEffect(() => {
|
||||
if (treeMode !== 'uncommitted') {
|
||||
|
|
|
|||
|
|
@ -1,5 +1,9 @@
|
|||
import { describe, expect, it } from 'vitest'
|
||||
import { getCombinedBranchEntries, getCombinedUncommittedEntries } from './combined-diff-entries'
|
||||
import {
|
||||
getCombinedBranchEntries,
|
||||
getCombinedUncommittedEntries,
|
||||
shouldAutoReloadCombinedDiffFromGitStatus
|
||||
} from './combined-diff-entries'
|
||||
import type { GitBranchChangeEntry, GitStatusEntry } from '../../../../shared/types'
|
||||
|
||||
describe('getCombinedUncommittedEntries', () => {
|
||||
|
|
@ -58,3 +62,38 @@ describe('getCombinedBranchEntries', () => {
|
|||
expect(getCombinedBranchEntries(undefined, liveEntries)).toEqual(liveEntries)
|
||||
})
|
||||
})
|
||||
|
||||
describe('shouldAutoReloadCombinedDiffFromGitStatus', () => {
|
||||
it('does not auto-reload snapshot-backed uncommitted diffs', () => {
|
||||
expect(
|
||||
shouldAutoReloadCombinedDiffFromGitStatus({
|
||||
mode: 'uncommitted',
|
||||
hasUncommittedEntriesSnapshot: true
|
||||
})
|
||||
).toBe(false)
|
||||
})
|
||||
|
||||
it('keeps the legacy live-entry uncommitted path reloadable', () => {
|
||||
expect(
|
||||
shouldAutoReloadCombinedDiffFromGitStatus({
|
||||
mode: 'uncommitted',
|
||||
hasUncommittedEntriesSnapshot: false
|
||||
})
|
||||
).toBe(true)
|
||||
})
|
||||
|
||||
it('does not use git status to reload branch or commit combined diffs', () => {
|
||||
expect(
|
||||
shouldAutoReloadCombinedDiffFromGitStatus({
|
||||
mode: 'branch',
|
||||
hasUncommittedEntriesSnapshot: false
|
||||
})
|
||||
).toBe(false)
|
||||
expect(
|
||||
shouldAutoReloadCombinedDiffFromGitStatus({
|
||||
mode: 'commit',
|
||||
hasUncommittedEntriesSnapshot: false
|
||||
})
|
||||
).toBe(false)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
import type { OpenFile } from '@/store/slices/editor'
|
||||
import type { GitBranchChangeEntry, GitStatusEntry } from '../../../../shared/types'
|
||||
import type { CombinedDiffFileTreeMode } from './combined-diff-file-tree-model'
|
||||
|
||||
/**
|
||||
* Fallback filtering for combined-diff tabs that were opened before the
|
||||
|
|
@ -29,3 +30,15 @@ export function getCombinedBranchEntries(
|
|||
// to later Source Control refreshes.
|
||||
return [...(snapshotEntries ?? liveEntries)]
|
||||
}
|
||||
|
||||
export function shouldAutoReloadCombinedDiffFromGitStatus({
|
||||
mode,
|
||||
hasUncommittedEntriesSnapshot
|
||||
}: {
|
||||
mode: CombinedDiffFileTreeMode
|
||||
hasUncommittedEntriesSnapshot: boolean
|
||||
}): boolean {
|
||||
// Why: snapshot-backed tabs intentionally preserve the tab-open diff while
|
||||
// staging/commit status churns; targeted editor-write reloads still refresh.
|
||||
return mode === 'uncommitted' && !hasUncommittedEntriesSnapshot
|
||||
}
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ import {
|
|||
isReloadableSingleFileDiffTab,
|
||||
shouldReloadDiffOnGitStatusChange
|
||||
} from './editor-panel-diff-reload'
|
||||
import type { GitStatusEntry } from '../../../../shared/types'
|
||||
|
||||
function makeDiffFile(overrides: Partial<OpenFile> = {}): OpenFile {
|
||||
return {
|
||||
|
|
@ -35,4 +36,34 @@ describe('editor-panel-diff-reload helpers', () => {
|
|||
expect(shouldReloadDiffOnGitStatusChange(makeDiffFile({ diffSource: 'branch' }))).toBe(false)
|
||||
expect(shouldReloadDiffOnGitStatusChange(makeDiffFile({ mode: 'edit' }))).toBe(false)
|
||||
})
|
||||
|
||||
it('keeps an unstaged diff snapshot when the row moves to staged', () => {
|
||||
const entries: GitStatusEntry[] = [{ path: 'file.ts', status: 'modified', area: 'staged' }]
|
||||
|
||||
expect(shouldReloadDiffOnGitStatusChange(makeDiffFile(), entries)).toBe(false)
|
||||
})
|
||||
|
||||
it('keeps a staged diff snapshot when the row is committed', () => {
|
||||
expect(shouldReloadDiffOnGitStatusChange(makeDiffFile({ diffSource: 'staged' }), [])).toBe(
|
||||
false
|
||||
)
|
||||
})
|
||||
|
||||
it('reloads a diff when its own status row is still present', () => {
|
||||
expect(
|
||||
shouldReloadDiffOnGitStatusChange(makeDiffFile(), [
|
||||
{ path: 'file.ts', status: 'modified', area: 'unstaged' }
|
||||
])
|
||||
).toBe(true)
|
||||
expect(
|
||||
shouldReloadDiffOnGitStatusChange(makeDiffFile(), [
|
||||
{ path: 'file.ts', status: 'untracked', area: 'untracked' }
|
||||
])
|
||||
).toBe(true)
|
||||
expect(
|
||||
shouldReloadDiffOnGitStatusChange(makeDiffFile({ diffSource: 'staged' }), [
|
||||
{ path: 'file.ts', status: 'modified', area: 'staged' }
|
||||
])
|
||||
).toBe(true)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -1,4 +1,5 @@
|
|||
import type { OpenFile } from '@/store/slices/editor'
|
||||
import type { GitStatusEntry } from '../../../../shared/types'
|
||||
|
||||
export function isReloadableSingleFileDiffTab(file: OpenFile): boolean {
|
||||
return (
|
||||
|
|
@ -10,6 +11,40 @@ export function isReloadableSingleFileDiffTab(file: OpenFile): boolean {
|
|||
)
|
||||
}
|
||||
|
||||
export function shouldReloadDiffOnGitStatusChange(file: OpenFile): boolean {
|
||||
return file.mode === 'diff' && (file.diffSource === 'unstaged' || file.diffSource === 'staged')
|
||||
function hasReloadableStatusEntry(
|
||||
file: OpenFile,
|
||||
gitStatusEntries: readonly GitStatusEntry[] | undefined
|
||||
): boolean {
|
||||
if (gitStatusEntries === undefined) {
|
||||
return true
|
||||
}
|
||||
|
||||
// Why: a diff tab snapshots one status area. If staging/commit moves that
|
||||
// row elsewhere, auto-reloading replaces useful context with another diff.
|
||||
if (file.diffSource === 'unstaged') {
|
||||
return gitStatusEntries.some(
|
||||
(entry) =>
|
||||
entry.path === file.relativePath &&
|
||||
(entry.area === 'unstaged' || entry.area === 'untracked')
|
||||
)
|
||||
}
|
||||
|
||||
if (file.diffSource === 'staged') {
|
||||
return gitStatusEntries.some(
|
||||
(entry) => entry.path === file.relativePath && entry.area === 'staged'
|
||||
)
|
||||
}
|
||||
|
||||
return true
|
||||
}
|
||||
|
||||
export function shouldReloadDiffOnGitStatusChange(
|
||||
file: OpenFile,
|
||||
gitStatusEntries?: readonly GitStatusEntry[]
|
||||
): boolean {
|
||||
return (
|
||||
file.mode === 'diff' &&
|
||||
(file.diffSource === 'unstaged' || file.diffSource === 'staged') &&
|
||||
hasReloadableStatusEntry(file, gitStatusEntries)
|
||||
)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -4,10 +4,12 @@ import { act } from 'react'
|
|||
import { createRoot, type Root } from 'react-dom/client'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type { OpenFile } from '@/store/slices/editor'
|
||||
import type { FileContent } from './editor-panel-content-types'
|
||||
import type { GitStatusEntry } from '../../../../shared/types'
|
||||
import type { DiffContent, FileContent } from './editor-panel-content-types'
|
||||
|
||||
const mocks = vi.hoisted(() => ({
|
||||
readRuntimeFileContent: vi.fn(),
|
||||
getRuntimeGitDiff: vi.fn(),
|
||||
getConnectionId: vi.fn(),
|
||||
getState: vi.fn()
|
||||
}))
|
||||
|
|
@ -26,7 +28,7 @@ vi.mock('@/runtime/runtime-file-client', () => ({
|
|||
vi.mock('@/runtime/runtime-git-client', () => ({
|
||||
getRuntimeGitBranchDiff: vi.fn(),
|
||||
getRuntimeGitCommitDiff: vi.fn(),
|
||||
getRuntimeGitDiff: vi.fn(),
|
||||
getRuntimeGitDiff: mocks.getRuntimeGitDiff,
|
||||
getRuntimeGitScope: vi.fn(() => null)
|
||||
}))
|
||||
|
||||
|
|
@ -45,18 +47,27 @@ import { useEditorPanelContentState } from './useEditorPanelContentState'
|
|||
type ProbeProps = {
|
||||
activeFile: OpenFile
|
||||
openFiles: OpenFile[]
|
||||
gitStatusByWorktree?: Record<string, GitStatusEntry[]>
|
||||
}
|
||||
|
||||
let latestFileContents: Record<string, FileContent> = {}
|
||||
let latestDiffContents: Record<string, DiffContent> = {}
|
||||
const EMPTY_GIT_STATUS_BY_WORKTREE: Record<string, GitStatusEntry[]> = {}
|
||||
|
||||
function HookProbe({ activeFile, openFiles }: ProbeProps): null {
|
||||
latestFileContents = useEditorPanelContentState({
|
||||
function HookProbe({
|
||||
activeFile,
|
||||
openFiles,
|
||||
gitStatusByWorktree = EMPTY_GIT_STATUS_BY_WORKTREE
|
||||
}: ProbeProps): null {
|
||||
const state = useEditorPanelContentState({
|
||||
activeFile,
|
||||
isChangesMode: false,
|
||||
openFiles,
|
||||
gitStatusByWorktree: {},
|
||||
gitStatusByWorktree,
|
||||
editorViewMode: {}
|
||||
}).fileContents
|
||||
})
|
||||
latestFileContents = state.fileContents
|
||||
latestDiffContents = state.diffContents
|
||||
return null
|
||||
}
|
||||
|
||||
|
|
@ -79,7 +90,9 @@ describe('useEditorPanelContentState', () => {
|
|||
|
||||
beforeEach(() => {
|
||||
latestFileContents = {}
|
||||
latestDiffContents = {}
|
||||
mocks.readRuntimeFileContent.mockReset()
|
||||
mocks.getRuntimeGitDiff.mockReset()
|
||||
mocks.getConnectionId.mockReset()
|
||||
mocks.getConnectionId.mockReturnValue(undefined)
|
||||
mocks.getState.mockReset()
|
||||
|
|
@ -126,4 +139,105 @@ describe('useEditorPanelContentState', () => {
|
|||
})
|
||||
)
|
||||
})
|
||||
|
||||
it('keeps a loaded unstaged diff when git status moves the row to staged', async () => {
|
||||
const activeFile = createOpenFile({
|
||||
id: 'wt-1::diff::unstaged::file.ts',
|
||||
mode: 'diff',
|
||||
diffSource: 'unstaged'
|
||||
})
|
||||
mocks.getRuntimeGitDiff.mockResolvedValue({
|
||||
kind: 'text',
|
||||
originalContent: 'old',
|
||||
modifiedContent: 'large diff content',
|
||||
originalIsBinary: false,
|
||||
modifiedIsBinary: false
|
||||
})
|
||||
|
||||
container = document.createElement('div')
|
||||
document.body.appendChild(container)
|
||||
root = createRoot(container)
|
||||
|
||||
await act(async () => {
|
||||
root?.render(
|
||||
<HookProbe
|
||||
activeFile={activeFile}
|
||||
openFiles={[activeFile]}
|
||||
gitStatusByWorktree={{
|
||||
'wt-1': [{ path: 'file.ts', status: 'modified', area: 'unstaged' }]
|
||||
}}
|
||||
/>
|
||||
)
|
||||
})
|
||||
|
||||
await vi.waitFor(() =>
|
||||
expect(latestDiffContents[activeFile.id]?.modifiedContent).toBe('large diff content')
|
||||
)
|
||||
|
||||
await act(async () => {
|
||||
root?.render(
|
||||
<HookProbe
|
||||
activeFile={activeFile}
|
||||
openFiles={[activeFile]}
|
||||
gitStatusByWorktree={{
|
||||
'wt-1': [{ path: 'file.ts', status: 'modified', area: 'staged' }]
|
||||
}}
|
||||
/>
|
||||
)
|
||||
})
|
||||
|
||||
expect(mocks.getRuntimeGitDiff).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('reloads a loaded unstaged diff when its own status row is still present', async () => {
|
||||
const activeFile = createOpenFile({
|
||||
id: 'wt-1::diff::unstaged::file.ts',
|
||||
mode: 'diff',
|
||||
diffSource: 'unstaged'
|
||||
})
|
||||
mocks.getRuntimeGitDiff
|
||||
.mockResolvedValueOnce({
|
||||
kind: 'text',
|
||||
originalContent: 'old',
|
||||
modifiedContent: 'first diff content',
|
||||
originalIsBinary: false,
|
||||
modifiedIsBinary: false
|
||||
})
|
||||
.mockResolvedValueOnce({
|
||||
kind: 'text',
|
||||
originalContent: 'old',
|
||||
modifiedContent: 'refreshed diff content',
|
||||
originalIsBinary: false,
|
||||
modifiedIsBinary: false
|
||||
})
|
||||
|
||||
container = document.createElement('div')
|
||||
document.body.appendChild(container)
|
||||
root = createRoot(container)
|
||||
|
||||
await act(async () => {
|
||||
root?.render(<HookProbe activeFile={activeFile} openFiles={[activeFile]} />)
|
||||
})
|
||||
|
||||
await vi.waitFor(() =>
|
||||
expect(latestDiffContents[activeFile.id]?.modifiedContent).toBe('first diff content')
|
||||
)
|
||||
|
||||
await act(async () => {
|
||||
root?.render(
|
||||
<HookProbe
|
||||
activeFile={activeFile}
|
||||
openFiles={[activeFile]}
|
||||
gitStatusByWorktree={{
|
||||
'wt-1': [{ path: 'file.ts', status: 'modified', area: 'unstaged' }]
|
||||
}}
|
||||
/>
|
||||
)
|
||||
})
|
||||
|
||||
await vi.waitFor(() =>
|
||||
expect(latestDiffContents[activeFile.id]?.modifiedContent).toBe('refreshed diff content')
|
||||
)
|
||||
expect(mocks.getRuntimeGitDiff).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -336,19 +336,31 @@ export function useEditorPanelContentState({
|
|||
const changesStatusEntries = activeFile?.worktreeId
|
||||
? gitStatusByWorktree[activeFile.worktreeId]
|
||||
: undefined
|
||||
const activeFileGitStatusSignature = useMemo(() => {
|
||||
const activeFileGitStatusEntries = useMemo(() => {
|
||||
if (!activeFile?.relativePath || !changesStatusEntries) {
|
||||
return undefined
|
||||
}
|
||||
return changesStatusEntries.filter((entry) => entry.path === activeFile.relativePath)
|
||||
}, [activeFile?.relativePath, changesStatusEntries])
|
||||
const activeFileGitStatusSignature = useMemo(() => {
|
||||
if (!activeFileGitStatusEntries) {
|
||||
return ''
|
||||
}
|
||||
const matching = changesStatusEntries.filter((entry) => entry.path === activeFile.relativePath)
|
||||
return JSON.stringify(
|
||||
matching.map((entry) => ({
|
||||
activeFileGitStatusEntries.map((entry) => ({
|
||||
area: entry.area,
|
||||
status: entry.status,
|
||||
conflictStatus: entry.conflictStatus
|
||||
}))
|
||||
)
|
||||
}, [activeFile?.relativePath, changesStatusEntries])
|
||||
}, [activeFileGitStatusEntries])
|
||||
const activeFileShouldReloadOnGitStatusChange = useMemo(
|
||||
() =>
|
||||
activeFile
|
||||
? shouldReloadDiffOnGitStatusChange(activeFile, activeFileGitStatusEntries)
|
||||
: false,
|
||||
[activeFile, activeFileGitStatusEntries]
|
||||
)
|
||||
useEffect(() => {
|
||||
if (!activeFile?.id) {
|
||||
return
|
||||
|
|
@ -357,7 +369,7 @@ export function useEditorPanelContentState({
|
|||
if (!current) {
|
||||
return
|
||||
}
|
||||
if (!(isChangesMode || shouldReloadDiffOnGitStatusChange(current))) {
|
||||
if (!(isChangesMode || activeFileShouldReloadOnGitStatusChange)) {
|
||||
return
|
||||
}
|
||||
// Why: the lazy-load effect already fetches on first open; forcing here
|
||||
|
|
@ -366,7 +378,13 @@ export function useEditorPanelContentState({
|
|||
return
|
||||
}
|
||||
void loadDiffContent(current, { force: true })
|
||||
}, [activeFileGitStatusSignature, isChangesMode, activeFile?.id, loadDiffContent])
|
||||
}, [
|
||||
activeFileShouldReloadOnGitStatusChange,
|
||||
activeFileGitStatusSignature,
|
||||
isChangesMode,
|
||||
activeFile?.id,
|
||||
loadDiffContent
|
||||
])
|
||||
|
||||
useEffect(() => {
|
||||
const nonce = activeFile?.diffContentReloadNonce
|
||||
|
|
|
|||
Loading…
Reference in New Issue