fix(editor): keep invalidated content visible and stop redundant reads

- Mark invalidated file content stale instead of dropping it so a reveal swaps
  bytes in place; the read-generation fence already blocks stale write-backs.
- Skip the lazy read when the newest read for the tab is still outstanding, so
  a worktree flip-flop cannot double-fetch.
- Stop claiming Cmd/Ctrl+S outside the workspace view, where no mounted panel
  owns the request and the keystroke was silently swallowed.
This commit is contained in:
Neil
2026-08-11 00:12:31 -07:00
parent 76713201ec
commit a633b951b3
5 changed files with 152 additions and 12 deletions
@@ -24,23 +24,57 @@ describe('getEditorCmdSaveFileId', () => {
expect(
getEditorCmdSaveFileId(
{ activeFileId: 'main-file', activeTabType: 'editor', getActiveTab },
{
activeFileId: 'main-file',
activeTabType: 'editor',
activeView: 'terminal',
getActiveTab
},
false
)
).toBe('main-file')
expect(getActiveTab).not.toHaveBeenCalled()
})
it('claims nothing on a non-workspace view so the shortcut is not swallowed', () => {
const getActiveTab = vi.fn(() => null)
for (const activeView of ['tasks', 'settings', 'activity'] as const) {
expect(
getEditorCmdSaveFileId(
{ activeFileId: 'main-file', activeTabType: 'editor', activeView, getActiveTab },
false
)
).toBeNull()
}
})
it('targets only an active floating editor and never falls through to main', () => {
const getActiveTab = vi
.fn<(worktreeId: string) => Tab | null>()
.mockReturnValueOnce(makeTab('editor', 'floating-file'))
.mockReturnValueOnce(makeTab('browser', 'floating-browser'))
const state = { activeFileId: 'main-file', activeTabType: 'editor', getActiveTab }
const state = {
activeFileId: 'main-file',
activeTabType: 'editor',
activeView: 'terminal' as const,
getActiveTab
}
expect(getEditorCmdSaveFileId(state, true)).toBe('floating-file')
expect(getEditorCmdSaveFileId(state, true)).toBeNull()
expect(getActiveTab).toHaveBeenNthCalledWith(1, FLOATING_TERMINAL_WORKTREE_ID)
expect(getActiveTab).toHaveBeenNthCalledWith(2, FLOATING_TERMINAL_WORKTREE_ID)
})
it('still targets the floating editor from a non-workspace view', () => {
const getActiveTab = vi.fn(() => makeTab('editor', 'floating-file'))
expect(
getEditorCmdSaveFileId(
{ activeFileId: 'main-file', activeTabType: 'editor', activeView: 'tasks', getActiveTab },
true
)
).toBe('floating-file')
})
})
@@ -1,5 +1,5 @@
import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../../shared/constants'
import type { Tab, TabContentType } from '../../../../shared/types'
import type { Tab, TabContentType, TopLevelView } from '../../../../shared/types'
export const EDITOR_TAB_CONTENT_TYPES = new Set<TabContentType>([
'editor',
@@ -11,6 +11,7 @@ export const EDITOR_TAB_CONTENT_TYPES = new Set<TabContentType>([
type EditorCmdSaveState = {
activeFileId: string | null
activeTabType: string | null
activeView: TopLevelView
getActiveTab: (worktreeId: string) => Tab | null
}
@@ -19,7 +20,12 @@ export function getEditorCmdSaveFileId(
floatingPanelOwnsEvent: boolean
): string | null {
if (!floatingPanelOwnsEvent) {
return state.activeTabType === 'editor' ? state.activeFileId : null
// Why: outside the workspace view no mounted panel claims the request, so
// returning an id would swallow Cmd/Ctrl+S on Tasks/Settings without saving.
// The floating panel floats above every view and keeps its own ownership.
return state.activeView === 'terminal' && state.activeTabType === 'editor'
? state.activeFileId
: null
}
const activeTab = state.getActiveTab(FLOATING_TERMINAL_WORKTREE_ID)
return activeTab && EDITOR_TAB_CONTENT_TYPES.has(activeTab.contentType)
@@ -25,6 +25,8 @@ export type FileContent = {
mimeType?: string
fileIdentity?: string
loadError?: string
/** Superseded by an external change; still rendered until the lazy reload lands. */
isStale?: boolean
}
export type DiffContent = GitDiffResult
@@ -85,6 +85,18 @@ function stampCleanTabDiskBaseline(id: string, result: FileContent): void {
}
}
// Why: the newest read for a tab is already on its way, so a re-run of the lazy
// effect (a worktree flip-flop re-adds `isVisible`) must not fire a second RPC.
// An invalidation bumps the tab's generation, so a superseded read never counts.
function hasLiveRead(
generationsById: Record<string, number>,
outstandingById: Record<string, number>,
id: string
): boolean {
const generation = generationsById[id]
return generation !== undefined && outstandingById[id] === generation
}
function inFlightReadKey(connectionId: string | undefined, filePath: string): string {
return `${connectionId ?? ''}::${filePath}`
}
@@ -123,6 +135,8 @@ export function useEditorPanelContentState({
const diffReadGenerationRef = useRef<Record<string, number>>({})
const fileReadGenerationCounterRef = useRef(0)
const diffReadGenerationCounterRef = useRef(0)
const outstandingFileReadsRef = useRef<Record<string, number>>({})
const outstandingDiffReadsRef = useRef<Record<string, number>>({})
const openFilesRef = useRef(openFiles)
const editorViewModeRef = useRef(editorViewMode)
const isVisibleRef = useRef(isVisible)
@@ -152,8 +166,11 @@ export function useEditorPanelContentState({
const next = { ...prev }
let changed = false
for (const fileId of uniqueIds) {
if (fileId in next) {
delete next[fileId]
const existing = next[fileId]
// Why: keep the last-known bytes rendered and swap them when the lazy
// reload lands — dropping them flashes "Loading…" on every reveal.
if (existing && existing.isStale !== true) {
next[fileId] = { ...existing, isStale: true }
changed = true
}
}
@@ -198,6 +215,7 @@ export function useEditorPanelContentState({
const generation = fileReadGenerationCounterRef.current + 1
fileReadGenerationCounterRef.current = generation
fileReadGenerationRef.current[id] = generation
outstandingFileReadsRef.current[id] = generation
try {
const resolvedConnectionId = getConnectionIdForFile(worktreeId ?? null, filePath)
const connectionId = resolvedConnectionId ?? undefined
@@ -326,6 +344,10 @@ export function useEditorPanelContentState({
...prev,
[id]: { content: '', isBinary: false, loadError: message }
}))
} finally {
if (outstandingFileReadsRef.current[id] === generation) {
delete outstandingFileReadsRef.current[id]
}
}
},
[]
@@ -339,6 +361,7 @@ export function useEditorPanelContentState({
const generation = diffReadGenerationCounterRef.current + 1
diffReadGenerationCounterRef.current = generation
diffReadGenerationRef.current[file.id] = generation
outstandingDiffReadsRef.current[file.id] = generation
try {
const worktreePath = file.filePath.slice(
0,
@@ -451,6 +474,10 @@ export function useEditorPanelContentState({
modifiedIsBinary: false
}
}))
} finally {
if (outstandingDiffReadsRef.current[file.id] === generation) {
delete outstandingDiffReadsRef.current[file.id]
}
}
},
[]
@@ -491,6 +518,17 @@ export function useEditorPanelContentState({
useLocalLogTail({ openFiles, fileContents, setFileContents, reloadContent })
const needsFileRead = (fileId: string): boolean => {
const cached = fileContents[fileId]
return (
(!cached || cached.isStale === true) &&
!hasLiveRead(fileReadGenerationRef.current, outstandingFileReadsRef.current, fileId)
)
}
const needsDiffRead = (fileId: string): boolean =>
!diffContents[fileId] &&
!hasLiveRead(diffReadGenerationRef.current, outstandingDiffReadsRef.current, fileId)
useEffect(() => {
if (!isVisible) {
return
@@ -514,7 +552,7 @@ export function useEditorPanelContentState({
}
const absolutePath = joinPath(activeFile.filePath, entry.path)
if (!fileContents[absolutePath]) {
if (needsFileRead(absolutePath)) {
void loadFileContent(absolutePath, absolutePath, activeFile.worktreeId, entry.path)
}
}
@@ -529,7 +567,7 @@ export function useEditorPanelContentState({
if (fileToLoad.conflict?.kind === 'conflict-placeholder') {
return
}
if (!fileContents[fileToLoad.id]) {
if (needsFileRead(fileToLoad.id)) {
void loadFileContent(
fileToLoad.filePath,
fileToLoad.id,
@@ -537,10 +575,10 @@ export function useEditorPanelContentState({
fileToLoad.relativePath
)
}
if (isChangesMode && !diffContents[fileToLoad.id]) {
if (isChangesMode && needsDiffRead(fileToLoad.id)) {
void loadDiffContent(fileToLoad)
}
} else if (isReloadableSingleFileDiffTab(fileToLoad) && !diffContents[fileToLoad.id]) {
} else if (isReloadableSingleFileDiffTab(fileToLoad) && needsDiffRead(fileToLoad.id)) {
void loadDiffContent(fileToLoad)
}
// oxlint-disable-next-line react-hooks/exhaustive-deps
@@ -217,6 +217,62 @@ describe('useEditorPanelContentState visibility', () => {
)
})
it('keeps invalidated bytes on screen until the reveal read lands', async () => {
const file = makeFile('no-flash')
const freshRead = createDeferred<FileContent>()
mocks.readRuntimeFileContent
.mockResolvedValueOnce({ content: 'old', isBinary: false })
.mockReturnValueOnce(freshRead.promise)
await act(async () => root.render(<Probe activeFile={file} />))
await vi.waitFor(() =>
expect(snapshots.get('main')?.fileContents[file.id]?.content).toBe('old')
)
await act(async () => root.render(<Probe activeFile={file} isVisible={false} />))
dispatchExternalChange(file)
expect(snapshots.get('main')?.fileContents[file.id]?.content).toBe('old')
await act(async () => root.render(<Probe activeFile={file} />))
await vi.waitFor(() => expect(mocks.readRuntimeFileContent).toHaveBeenCalledTimes(2))
// Why: an unresolved reveal read must not blank the pane to "Loading…".
expect(snapshots.get('main')?.fileContents[file.id]?.content).toBe('old')
await act(async () => {
freshRead.resolve({ content: 'fresh', isBinary: false })
await freshRead.promise
})
await vi.waitFor(() =>
expect(snapshots.get('main')?.fileContents[file.id]).toEqual({
content: 'fresh',
isBinary: false
})
)
})
it('does not re-read while the reveal read is still in flight', async () => {
const file = makeFile('flip-flop')
const pendingRead = createDeferred<FileContent>()
mocks.readRuntimeFileContent.mockReturnValue(pendingRead.promise)
await act(async () => root.render(<Probe activeFile={file} isVisible={false} />))
await act(async () => root.render(<Probe activeFile={file} />))
expect(mocks.readRuntimeFileContent).toHaveBeenCalledOnce()
await act(async () => root.render(<Probe activeFile={file} isVisible={false} />))
await act(async () => root.render(<Probe activeFile={file} />))
expect(mocks.readRuntimeFileContent).toHaveBeenCalledOnce()
await act(async () => {
pendingRead.resolve({ content: 'remote', isBinary: false })
await pendingRead.promise
})
await vi.waitFor(() =>
expect(snapshots.get('main')?.fileContents[file.id]?.content).toBe('remote')
)
expect(mocks.readRuntimeFileContent).toHaveBeenCalledOnce()
})
it('publishes hidden visibility before a watcher event in the same commit', async () => {
const file = makeFile('commit-visibility')
mocks.readRuntimeFileContent.mockResolvedValue({ content: 'old', isBinary: false })
@@ -234,7 +290,11 @@ describe('useEditorPanelContentState visibility', () => {
)
expect(mocks.readRuntimeFileContent).toHaveBeenCalledOnce()
expect(snapshots.get('main')?.fileContents[file.id]).toBeUndefined()
expect(snapshots.get('main')?.fileContents[file.id]).toEqual({
content: 'old',
isBinary: false,
isStale: true
})
})
it('shares one post-change read between visible source and preview panels', async () => {
@@ -285,7 +345,7 @@ describe('useEditorPanelContentState visibility', () => {
const changed = { ...file, fileContentReloadNonce: 1 }
await act(async () => root.render(<Probe activeFile={changed} isVisible={false} />))
expect(snapshots.get('main')?.fileContents[file.id]).toBeUndefined()
expect(snapshots.get('main')?.fileContents[file.id]?.isStale).toBe(true)
expect(mocks.readRuntimeFileContent).toHaveBeenCalledOnce()
await act(async () => root.render(<Probe activeFile={changed} />))