mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 08:03:37 +00:00
fix(editor): stop a failed diff load from being saved over the file
When a diff RPC fails, useEditorPanelContentState fabricates a result whose
modifiedContent is \`Error loading diff: \${err}\` so the pane has something to
show. For an unstaged diff that pane is editable, so Cmd+S calls onSave with the
editor body — and attemptEditorFileSave falls back to that body when no draft
exists (editor-autosave-controller). The error sentence gets written over the
user's file.
Reachable from any diff-load failure: SSH drop, git error, permission denied, or
the diff_too_large introduced in #14160.
The existing largeDiffSaveContentAvailable guard does not cover this. It only
gates the LargeDiffFallback button, which renders when largeDiffRenderLimit is
limited; on the error path that field is absent, so the full editable DiffEditor
mounts and installs the save shortcut instead.
Marks the fabricated result and holds the pane read-only, which blocks the save
shortcut and prevents a draft being minted — a draft would otherwise be saveable
even with onSave gated. loadError mirrors the field FileContent already carries
for the plain-file path, and lives on the renderer-local DiffContent, so nothing
on the wire changes.
Test: mutation-verified — dropping the guard fails exactly the new read-only
case. 23094 renderer tests pass; web and node typechecks clean.
Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -0,0 +1,126 @@
|
||||
// @vitest-environment happy-dom
|
||||
// Why: a failed diff load renders the error text as the modified body. If that pane stays editable
|
||||
// the save path writes it over the real file, because attemptEditorFileSave falls back to the body
|
||||
// shown when no draft exists.
|
||||
import { cleanup, render } from '@testing-library/react'
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
import type { OpenFile } from '@/store/slices/editor'
|
||||
import type { DiffContent } from './editor-panel-content-types'
|
||||
|
||||
const captured = vi.hoisted(() => ({
|
||||
diffProps: [] as { editable?: boolean; onSave?: unknown; onContentChange?: unknown }[]
|
||||
}))
|
||||
|
||||
vi.mock('@/lib/lazy-with-retry', () => ({
|
||||
lazyWithRetry: (factory: () => Promise<unknown>) => {
|
||||
if (factory.toString().includes('/DiffViewer.tsx')) {
|
||||
return function MockDiffViewer(props: {
|
||||
editable?: boolean
|
||||
onSave?: unknown
|
||||
onContentChange?: unknown
|
||||
}) {
|
||||
captured.diffProps.push(props)
|
||||
return null
|
||||
}
|
||||
}
|
||||
return () => null
|
||||
}
|
||||
}))
|
||||
|
||||
vi.mock('@/store', () => {
|
||||
const state = {
|
||||
worktreesByRepo: {},
|
||||
openFile: vi.fn(),
|
||||
openMarkdownPreview: vi.fn(),
|
||||
openConflictReviewFile: vi.fn(),
|
||||
openConflictReview: vi.fn(),
|
||||
closeFile: vi.fn(),
|
||||
setRightSidebarTab: vi.fn(),
|
||||
setPendingEditorReveal: vi.fn(),
|
||||
reloadOpenCheckRunDetailsTab: vi.fn()
|
||||
}
|
||||
return {
|
||||
useAppStore: Object.assign(
|
||||
(selector: (s: Record<string, unknown>) => unknown) => selector(state),
|
||||
{ getState: () => ({ ...state, folderWorkspaces: [], projectGroups: [], repos: [] }) }
|
||||
)
|
||||
}
|
||||
})
|
||||
|
||||
import { EditorContent } from './EditorContent'
|
||||
|
||||
const ACTIVE_FILE: OpenFile = {
|
||||
id: '/repo/notes.ts',
|
||||
filePath: '/repo/notes.ts',
|
||||
relativePath: 'notes.ts',
|
||||
worktreeId: 'repo::/repo',
|
||||
language: 'typescript',
|
||||
isDirty: false,
|
||||
mode: 'diff',
|
||||
diffSource: 'unstaged'
|
||||
}
|
||||
|
||||
function renderDiff(diff: DiffContent): void {
|
||||
render(
|
||||
<EditorContent
|
||||
activeFile={ACTIVE_FILE}
|
||||
viewStateScopeId={ACTIVE_FILE.id}
|
||||
fileContents={{}}
|
||||
diffContents={{ [ACTIVE_FILE.id]: diff }}
|
||||
editBuffers={{}}
|
||||
openFiles={[ACTIVE_FILE]}
|
||||
worktreeEntries={[]}
|
||||
resolvedLanguage="typescript"
|
||||
isMarkdown={false}
|
||||
isMermaid={false}
|
||||
isCsv={false}
|
||||
isNotebook={false}
|
||||
mdViewMode="rich"
|
||||
isChangesMode={false}
|
||||
sideBySide={false}
|
||||
pendingEditorReveal={null}
|
||||
handleContentChange={vi.fn()}
|
||||
handleContentChangeForFile={vi.fn()}
|
||||
handleDirtyStateHint={vi.fn()}
|
||||
handleSave={vi.fn()}
|
||||
handleSaveForFile={vi.fn()}
|
||||
reloadContent={vi.fn()}
|
||||
/>
|
||||
)
|
||||
}
|
||||
|
||||
function textDiff(modifiedContent: string, loadError?: boolean): DiffContent {
|
||||
return {
|
||||
kind: 'text',
|
||||
originalContent: '',
|
||||
modifiedContent,
|
||||
originalIsBinary: false,
|
||||
modifiedIsBinary: false,
|
||||
...(loadError === undefined ? {} : { loadError })
|
||||
}
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
cleanup()
|
||||
captured.diffProps.length = 0
|
||||
})
|
||||
|
||||
describe('EditorContent diff load failures', () => {
|
||||
it('keeps a failed diff load read-only so its message cannot be saved over the file', () => {
|
||||
renderDiff(textDiff('Error loading diff: RuntimeRpcCallError: too large', true))
|
||||
|
||||
const props = captured.diffProps.at(-1)
|
||||
expect(props?.editable).toBe(false)
|
||||
expect(props?.onSave).toBeUndefined()
|
||||
// Why: no draft can be minted either, or the next save would write the typed-over message.
|
||||
expect(props?.onContentChange).toBeUndefined()
|
||||
})
|
||||
|
||||
it('leaves a normally loaded unstaged diff editable', () => {
|
||||
renderDiff(textDiff('export const a = 1\n'))
|
||||
|
||||
const props = captured.diffProps.at(-1)
|
||||
expect(props?.editable).toBe(true)
|
||||
expect(props?.onSave).toBeTypeOf('function')
|
||||
})
|
||||
})
|
||||
@@ -14,7 +14,7 @@ import {
|
||||
getNextConflictNavigationIndex
|
||||
} from './ConflictComponents'
|
||||
import type { MarkdownViewMode, OpenFile, PendingEditorReveal } from '@/store/slices/editor'
|
||||
import type { GitDiffResult } from '../../../../shared/git-diff-compare-types'
|
||||
import type { DiffContent } from './editor-panel-content-types'
|
||||
import type { GitStatusEntry } from '../../../../shared/git-status-types'
|
||||
import { getMarkdownRenderMode } from './markdown-render-mode'
|
||||
import { getMarkdownRichModeUnsupportedMessage } from './markdown-rich-mode'
|
||||
@@ -147,7 +147,7 @@ export function EditorContent({
|
||||
activeFile: OpenFile
|
||||
viewStateScopeId: string
|
||||
fileContents: Record<string, FileContent>
|
||||
diffContents: Record<string, GitDiffResult>
|
||||
diffContents: Record<string, DiffContent>
|
||||
editBuffers: Record<string, string>
|
||||
openFiles: OpenFile[]
|
||||
worktreeEntries: GitStatusEntry[]
|
||||
@@ -895,6 +895,9 @@ export function EditorContent({
|
||||
modifiedDiffBuffer === undefined &&
|
||||
dc.modifiedContent.length === 0
|
||||
)
|
||||
// Why: the body is a load-failure message, not file content. Editing must stay off so no draft
|
||||
// is minted and no save shortcut is installed — either would write that text over the real file.
|
||||
const diffEditable = isEditable && dc.loadError !== true
|
||||
// Why: shared by both diff sub-branches (preview and source) so preview mode surfaces the external change too.
|
||||
const diffExternalChangeBanner =
|
||||
activeFile.externalMutation === 'changed' ? (
|
||||
@@ -952,10 +955,10 @@ export function EditorContent({
|
||||
filePath={activeFile.filePath}
|
||||
relativePath={activeFile.relativePath}
|
||||
sideBySide={sideBySide}
|
||||
editable={isEditable}
|
||||
editable={diffEditable}
|
||||
worktreeId={activeFile.worktreeId}
|
||||
onContentChange={isEditable ? handleContentChange : undefined}
|
||||
onSave={isEditable ? (isMarkdown ? md.mdSave : handleSave) : undefined}
|
||||
onContentChange={diffEditable ? handleContentChange : undefined}
|
||||
onSave={diffEditable ? (isMarkdown ? md.mdSave : handleSave) : undefined}
|
||||
/>
|
||||
)
|
||||
// Why: editable diffs get the changed-on-disk banner; its reload refetches the diff body, not plain file content.
|
||||
|
||||
@@ -32,6 +32,8 @@ export type FileContent = {
|
||||
export type DiffContent = GitDiffResult & {
|
||||
/** Superseded by an external change; still rendered until the lazy reload lands. */
|
||||
isStale?: boolean
|
||||
/** The body is a load-failure message, not file content — saving it would overwrite the file. */
|
||||
loadError?: boolean
|
||||
}
|
||||
|
||||
export type InFlightContentRead<T> = {
|
||||
|
||||
@@ -167,7 +167,8 @@ export function useEditorPanelDiffContentLoader({
|
||||
originalContent: '',
|
||||
modifiedContent: `Error loading diff: ${String(err)}`,
|
||||
originalIsBinary: false,
|
||||
modifiedIsBinary: false
|
||||
modifiedIsBinary: false,
|
||||
loadError: true
|
||||
}
|
||||
}))
|
||||
} finally {
|
||||
|
||||
Reference in New Issue
Block a user