fix: address review findings (#2999)

This commit is contained in:
Jinjing 2026-05-28 11:00:33 -07:00 committed by GitHub
parent 0c5ed1e644
commit cf2303d471
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 559 additions and 25 deletions

View File

@ -1,4 +1,4 @@
import { useEffect, useMemo, useState } from 'react'
import { useEffect, useMemo, useRef, useState } from 'react'
import {
Columns2,
Copy,
@ -7,6 +7,7 @@ import {
FileText,
ListTree,
MoreHorizontal,
Pencil,
Rows2
} from 'lucide-react'
import { useAppStore } from '@/store'
@ -19,6 +20,7 @@ import {
DropdownMenuShortcut,
DropdownMenuTrigger
} from '@/components/ui/dropdown-menu'
import { Input } from '@/components/ui/input'
import { Tooltip, TooltipContent, TooltipProvider, TooltipTrigger } from '@/components/ui/tooltip'
import { CLOSE_ALL_CONTEXT_MENUS_EVENT } from '../tab-bar/SortableTab'
import { useShortcutLabel } from '@/hooks/useShortcutLabel'
@ -30,6 +32,7 @@ import type { EditorToggleValue } from './EditorViewToggle'
import type { EditorHeaderOpenFileState } from './editor-header'
import { getEditorHeaderCopyState } from './editor-header'
import { DiffNotesSendMenu } from './DiffNotesSendMenu'
import { useEditorHeaderFileRename } from './editor-header-file-rename'
const isMac = navigator.userAgent.includes('Mac')
const isLinux = navigator.userAgent.includes('Linux')
@ -104,7 +107,17 @@ export function EditorPanelHeader({
}: EditorPanelHeaderProps): React.JSX.Element {
const [pathMenuOpen, setPathMenuOpen] = useState(false)
const [pathMenuPoint, setPathMenuPoint] = useState({ x: 0, y: 0 })
const skipMenuFocusRestoreRef = useRef(false)
const headerCopyState = getEditorHeaderCopyState(activeFile)
const {
canRename,
currentFileName,
isRenaming,
renameInputRef,
openRenameInput,
commitRename,
cancelRename
} = useEditorHeaderFileRename(activeFile)
const diffComments = useAppStore((s) => s.getDiffComments(activeFile.worktreeId))
const activeGroupId = useAppStore((s) => s.activeGroupIdByWorktree[activeFile.worktreeId])
const fileDiffComments = useMemo(
@ -131,14 +144,43 @@ export function EditorPanelHeader({
setPathMenuOpen(true)
}}
>
<button
type="button"
className="editor-header-path"
onClick={onCopyPath}
title={headerCopyState.pathTitle}
>
{headerCopyState.pathLabel}
</button>
{isRenaming ? (
<Input
ref={renameInputRef}
data-editor-header-rename-input="true"
aria-label={`Rename file ${currentFileName}`}
defaultValue={currentFileName}
// Why: the header is narrow in floating mode; this keeps the
// edit field aligned with the path label without growing chrome.
className="h-6 w-[16ch] min-w-[104px] max-w-full rounded-sm bg-input/40 px-1.5 py-0 font-mono text-xs text-foreground md:text-xs focus-visible:ring-[1px]"
spellCheck={false}
onPointerDown={(event) => event.stopPropagation()}
onMouseDown={(event) => event.stopPropagation()}
onClick={(event) => event.stopPropagation()}
onDoubleClick={(event) => event.stopPropagation()}
onKeyDown={(event) => {
if (event.key === 'Enter') {
event.preventDefault()
event.stopPropagation()
commitRename()
} else if (event.key === 'Escape') {
event.preventDefault()
event.stopPropagation()
cancelRename()
}
}}
onBlur={commitRename}
/>
) : (
<button
type="button"
className="editor-header-path"
onClick={onCopyPath}
title={headerCopyState.pathTitle}
>
{headerCopyState.pathLabel}
</button>
)}
<span
className={`editor-header-copy-toast${copiedPathVisible ? ' is-visible' : ''}`}
aria-live="polite"
@ -155,7 +197,29 @@ export function EditorPanelHeader({
style={{ left: pathMenuPoint.x, top: pathMenuPoint.y }}
/>
</DropdownMenuTrigger>
<DropdownMenuContent className="w-56" sideOffset={0} align="start">
<DropdownMenuContent
className="w-56"
sideOffset={0}
align="start"
onCloseAutoFocus={(event) => {
if (!skipMenuFocusRestoreRef.current) {
return
}
skipMenuFocusRestoreRef.current = false
event.preventDefault()
}}
>
<DropdownMenuItem
disabled={!canRename}
onSelect={() => {
skipMenuFocusRestoreRef.current = true
openRenameInput()
}}
>
<Pencil className="w-3.5 h-3.5 mr-1.5" />
Rename
</DropdownMenuItem>
<DropdownMenuSeparator />
<DropdownMenuItem
onSelect={() => {
void window.api.ui.writeClipboardText(activeFile.filePath)

View File

@ -0,0 +1,94 @@
import { useEffect, useRef, useState } from 'react'
import type { OpenFile } from '@/store/slices/editor'
import { useWorktreeById } from '@/store/selectors'
import { basename } from '@/lib/path'
import { renameFileOnDisk } from '@/lib/rename-file'
import { getUntitledFileRoot } from './untitled-file-rename-path'
type EditorHeaderFileRenameState = {
canRename: boolean
currentFileName: string
isRenaming: boolean
renameInputRef: React.RefObject<HTMLInputElement | null>
openRenameInput: () => void
commitRename: () => void
cancelRename: () => void
}
export function useEditorHeaderFileRename(activeFile: OpenFile): EditorHeaderFileRenameState {
const worktree = useWorktreeById(activeFile.worktreeId)
const [isRenaming, setIsRenaming] = useState(false)
const renameInputRef = useRef<HTMLInputElement>(null)
const renameCancelledRef = useRef(false)
const currentFileName = basename(activeFile.filePath)
const canRename =
activeFile.mode === 'edit' && !activeFile.diffSource && !activeFile.conflict && !isRenaming
const openRenameInput = (): void => {
if (!canRename) {
return
}
renameCancelledRef.current = false
setIsRenaming(true)
}
const commitRename = (): void => {
if (renameCancelledRef.current) {
renameCancelledRef.current = false
setIsRenaming(false)
return
}
const input = renameInputRef.current
if (!input) {
setIsRenaming(false)
return
}
const newName = input.value.trim()
setIsRenaming(false)
if (!newName || newName === currentFileName) {
return
}
const worktreePath = getUntitledFileRoot(activeFile, worktree?.path ?? null)
void renameFileOnDisk({
oldPath: activeFile.filePath,
newName,
worktreeId: activeFile.worktreeId,
worktreePath
})
}
const cancelRename = (): void => {
renameCancelledRef.current = true
setIsRenaming(false)
}
useEffect(() => {
if (!isRenaming) {
return
}
const raf = requestAnimationFrame(() => {
const el = renameInputRef.current
if (!el) {
return
}
el.focus()
const dotIndex = currentFileName.lastIndexOf('.')
if (dotIndex > 0) {
el.setSelectionRange(0, dotIndex)
} else {
el.select()
}
})
return () => cancelAnimationFrame(raf)
}, [currentFileName, isRenaming])
return {
canRename,
currentFileName,
isRenaming,
renameInputRef,
openRenameInput,
commitRename,
cancelRename
}
}

View File

@ -0,0 +1,307 @@
import { beforeEach, describe, expect, it, vi } from 'vitest'
import type { OpenFile } from '../../store/slices/editor'
const reactHookRuntime = vi.hoisted(() => ({
states: [] as unknown[],
index: 0
}))
const appStoreMocks = vi.hoisted(() => ({
openMarkdownPreview: vi.fn(),
getState: vi.fn(() => ({
settings: {}
}))
}))
vi.mock('react', async () => {
const actual = await vi.importActual<typeof import('react')>('react') // eslint-disable-line @typescript-eslint/consistent-type-imports -- vi.importActual requires inline import()
return {
...actual,
useEffect: () => {},
useRef<T>(initial: T) {
return { current: initial }
},
useState<T>(initial: T | (() => T)) {
const stateIndex = reactHookRuntime.index++
if (!(stateIndex in reactHookRuntime.states)) {
reactHookRuntime.states[stateIndex] =
typeof initial === 'function' ? (initial as () => T)() : initial
}
const setState = (next: T | ((previous: T) => T)): void => {
reactHookRuntime.states[stateIndex] =
typeof next === 'function'
? (next as (previous: T) => T)(reactHookRuntime.states[stateIndex] as T)
: next
}
return [reactHookRuntime.states[stateIndex] as T, setState] as const
}
}
})
vi.mock('@dnd-kit/sortable', () => ({
useSortable: () => ({
attributes: {},
listeners: { onPointerDown: vi.fn() },
setNodeRef: vi.fn()
})
}))
vi.mock('lucide-react', () => ({
Columns2: function Columns2(props: Record<string, unknown>) {
return { type: 'Columns2', props }
},
Copy: function Copy(props: Record<string, unknown>) {
return { type: 'Copy', props }
},
ExternalLink: function ExternalLink(props: Record<string, unknown>) {
return { type: 'ExternalLink', props }
},
Eye: function Eye(props: Record<string, unknown>) {
return { type: 'Eye', props }
},
GitCompareArrows: function GitCompareArrows(props: Record<string, unknown>) {
return { type: 'GitCompareArrows', props }
},
Pencil: function Pencil(props: Record<string, unknown>) {
return { type: 'Pencil', props }
},
Rows2: function Rows2(props: Record<string, unknown>) {
return { type: 'Rows2', props }
},
ShieldAlert: function ShieldAlert(props: Record<string, unknown>) {
return { type: 'ShieldAlert', props }
},
X: function X(props: Record<string, unknown>) {
return { type: 'X', props }
}
}))
vi.mock('@/components/ui/dropdown-menu', () => ({
DropdownMenu: function DropdownMenu(props: { children?: unknown }) {
return { type: 'DropdownMenu', props }
},
DropdownMenuContent: function DropdownMenuContent(props: { children?: unknown }) {
return { type: 'DropdownMenuContent', props }
},
DropdownMenuItem: function DropdownMenuItem(props: { children?: unknown }) {
return { type: 'DropdownMenuItem', props }
},
DropdownMenuSeparator: function DropdownMenuSeparator() {
return { type: 'DropdownMenuSeparator', props: {} }
},
DropdownMenuTrigger: function DropdownMenuTrigger(props: { children?: unknown }) {
return { type: 'DropdownMenuTrigger', props }
}
}))
vi.mock('@/components/ui/input', () => ({
Input: function Input(props: Record<string, unknown>) {
return { type: 'input', props }
}
}))
vi.mock('@/components/editor/editor-labels', () => ({
getEditorDisplayLabel: (file: OpenFile) => file.relativePath
}))
vi.mock('@/lib/rename-file', () => ({
renameFileOnDisk: vi.fn()
}))
vi.mock('@/lib/file-type-icons', () => ({
getFileTypeIcon: () =>
function FileIcon(props: Record<string, unknown>) {
return { type: 'FileIcon', props }
}
}))
vi.mock('@/store/selectors', () => ({
useRepoById: () => ({ connectionId: null }),
useWorktreeById: () => ({ path: '/repo', repoId: 'repo-1' })
}))
vi.mock('@/store', () => {
const useAppStore = (selector: (state: { openMarkdownPreview: typeof vi.fn }) => unknown) =>
selector({ openMarkdownPreview: appStoreMocks.openMarkdownPreview })
useAppStore.getState = appStoreMocks.getState
return { useAppStore }
})
vi.mock('../right-sidebar/status-display', () => ({
STATUS_COLORS: {},
STATUS_LABELS: {}
}))
vi.mock('./SortableTab', () => ({
CLOSE_ALL_CONTEXT_MENUS_EVENT: 'orca-close-all-context-menus'
}))
vi.mock('./drop-indicator', () => ({
ACTIVE_TAB_INDICATOR_CLASSES: 'active-tab-indicator',
getDropIndicatorClasses: () => ''
}))
vi.mock('@/components/editor/markdown-preview-controls', () => ({
canOpenMarkdownPreview: () => false
}))
vi.mock('@/lib/local-path-open-guard', () => ({
shouldBlockEditorTabLocalOpen: () => false,
showLocalPathOpenBlockedToast: vi.fn()
}))
type ReactElementLike = {
type: unknown
props: Record<string, unknown>
}
function baseFile(overrides: Partial<OpenFile> = {}): OpenFile {
return {
id: '/repo/untitled-5.md',
filePath: '/repo/untitled-5.md',
relativePath: 'untitled-5.md',
worktreeId: 'wt-1',
language: 'markdown',
isDirty: false,
mode: 'edit',
...overrides
}
}
async function renderEditorFileTab(
file: OpenFile,
onActivate = vi.fn()
): Promise<{ element: unknown; onActivate: ReturnType<typeof vi.fn> }> {
reactHookRuntime.index = 0
const module = await import('./EditorFileTab')
const element = module.default({
file,
isActive: true,
hasTabsToRight: false,
statusByRelativePath: new Map(),
onActivate,
onClose: () => {},
onCloseToRight: () => {},
onCloseAll: () => {},
onPin: () => {},
onSplitGroup: () => {},
dragData: {
kind: 'tab',
worktreeId: file.worktreeId,
groupId: 'group-1',
unifiedTabId: file.id,
visibleTabId: file.id,
tabType: 'editor',
label: file.relativePath,
iconPath: file.filePath
}
})
return { element, onActivate }
}
function expandNode(node: unknown): unknown {
if (node == null || typeof node === 'string' || typeof node === 'number') {
return node
}
if (Array.isArray(node)) {
return node.map(expandNode)
}
const el = node as ReactElementLike
if (typeof el.type === 'function') {
return expandNode(el.type(el.props))
}
return {
...el,
props: {
...el.props,
children: expandNode(el.props?.children)
}
}
}
function getText(node: unknown): string {
if (node == null) {
return ''
}
if (typeof node === 'string' || typeof node === 'number') {
return String(node)
}
if (Array.isArray(node)) {
return node.map(getText).join('')
}
const el = node as ReactElementLike
return getText(el.props?.children)
}
function findElementsByType(node: unknown, typeName: string): ReactElementLike[] {
const results: ReactElementLike[] = []
const visit = (current: unknown): void => {
if (current == null || typeof current === 'string' || typeof current === 'number') {
return
}
if (Array.isArray(current)) {
for (const child of current) {
visit(child)
}
return
}
const el = current as ReactElementLike
if (el.type === typeName) {
results.push(el)
}
visit(el.props?.children)
}
visit(node)
return results
}
function findMenuItemByText(node: unknown, label: string): ReactElementLike {
const item = findElementsByType(node, 'DropdownMenuItem').find((candidate) =>
getText(candidate).includes(label)
)
if (!item) {
throw new Error(`Missing menu item: ${label}`)
}
return item
}
describe('EditorFileTab rename menu', () => {
beforeEach(() => {
reactHookRuntime.states = []
reactHookRuntime.index = 0
vi.clearAllMocks()
vi.resetModules()
vi.stubGlobal('navigator', { userAgent: 'Mac' })
})
it('turns the tab filename into an inline input from the Rename context-menu item', async () => {
const onActivate = vi.fn()
const file = baseFile({ isUntitled: true })
const firstRender = expandNode((await renderEditorFileTab(file, onActivate)).element)
const renameItem = findMenuItemByText(firstRender, 'Rename')
// Why: New Markdown tabs are real on-disk files even while marked
// isUntitled; the tab menu must let users rename the screenshot-style
// "untitled-N.md" files directly.
expect(renameItem.props.disabled).toBe(false)
;(renameItem.props.onSelect as () => void)()
const secondRender = expandNode((await renderEditorFileTab(file, onActivate)).element)
const inputs = findElementsByType(secondRender, 'input')
expect(inputs).toHaveLength(1)
expect(inputs[0].props.defaultValue).toBe('untitled-5.md')
expect(inputs[0].props['data-tab-rename-input']).toBe('true')
expect(onActivate).toHaveBeenCalledTimes(1)
})
it('disables Rename for diff tabs that do not map to one writable file', async () => {
const file = baseFile({
mode: 'diff',
diffSource: 'unstaged'
})
const element = expandNode((await renderEditorFileTab(file)).element)
const renameItem = findMenuItemByText(element, 'Rename')
expect(renameItem.props.disabled).toBe(true)
})
})

View File

@ -9,7 +9,8 @@ import {
ShieldAlert,
ExternalLink,
Columns2,
Rows2
Rows2,
Pencil
} from 'lucide-react'
import {
DropdownMenu,
@ -18,6 +19,7 @@ import {
DropdownMenuSeparator,
DropdownMenuTrigger
} from '@/components/ui/dropdown-menu'
import { Input } from '@/components/ui/input'
import { basename, normalizeRelativePath } from '@/lib/path'
import { getEditorDisplayLabel } from '@/components/editor/editor-labels'
import { renameFileOnDisk } from '@/lib/rename-file'
@ -28,6 +30,7 @@ import { useAppStore } from '@/store'
import { STATUS_COLORS, STATUS_LABELS } from '../right-sidebar/status-display'
import type { GitFileStatus } from '../../../../shared/types'
import type { OpenFile } from '../../store/slices/editor'
import { getUntitledFileRoot } from '@/components/editor/untitled-file-rename-path'
import { preventMiddleButtonDefault } from './middle-button-default-guard'
import { CLOSE_ALL_CONTEXT_MENUS_EVENT } from './SortableTab'
import type { TabDragItemData } from '../tab-group/useTabDragSplit'
@ -109,15 +112,23 @@ export default function EditorFileTab({
const [menuPoint, setMenuPoint] = useState({ x: 0, y: 0 })
const [isRenaming, setIsRenaming] = useState(false)
const renameInputRef = useRef<HTMLInputElement>(null)
const skipMenuFocusRestoreRef = useRef(false)
// Escape fires setIsRenaming(false), which unmounts the input. The browser
// still fires focusout as the focused node is removed, so onBlur can invoke
// commitRename *after* cancel — committing the typed value against the
// user's intent. This flag suppresses the trailing blur-commit.
const renameCancelledRef = useRef(false)
// Only real on-disk files in edit mode are renameable. Diff, conflict-review,
// untitled drafts, and combined/virtual views don't point at a single concrete
// file we can safely rename.
const canRename = file.mode === 'edit' && !file.isUntitled && !file.diffSource && !file.conflict
// Only on-disk edit tabs are renameable. Diff, conflict-review, and
// combined/virtual views don't point at a single concrete file we can safely rename.
const canRename = file.mode === 'edit' && !file.diffSource && !file.conflict
const openRenameInput = (): void => {
if (!canRename) {
return
}
renameCancelledRef.current = false
setIsRenaming(true)
}
const commitRename = (): void => {
if (renameCancelledRef.current) {
@ -139,10 +150,7 @@ export default function EditorFileTab({
if (newName === oldName) {
return
}
const worktreePath = worktree?.path ?? null
if (!worktreePath) {
return
}
const worktreePath = getUntitledFileRoot(file, worktree?.path ?? null)
void renameFileOnDisk({
oldPath: file.filePath,
newName,
@ -261,12 +269,15 @@ export default function EditorFileTab({
)}
<span className="mr-1 flex min-w-0 items-baseline gap-1">
{isRenaming ? (
<input
<Input
ref={renameInputRef}
data-tab-rename-input="true"
aria-label={`Rename file ${basename(file.filePath)}`}
defaultValue={basename(file.filePath)}
// Tiny border to make the edit affordance obvious without
// changing overall tab height. Size matches the label span.
className="truncate max-w-[80px] bg-transparent text-xs text-foreground outline-none border border-ring rounded-sm px-1 py-0"
// Why: keep the inline field compact enough for the titlebar while
// giving filenames a little more room than the static tab label.
className="mr-1 h-5 w-[12ch] min-w-[72px] max-w-[132px] rounded-sm bg-input/40 px-1 py-0 text-xs text-foreground md:text-xs focus-visible:ring-[1px]"
spellCheck={false}
onPointerDown={(e) => e.stopPropagation()}
onMouseDown={(e) => e.stopPropagation()}
onClick={(e) => e.stopPropagation()}
@ -297,7 +308,7 @@ export default function EditorFileTab({
return
}
e.stopPropagation()
setIsRenaming(true)
openRenameInput()
}}
>
{getEditorDisplayLabel(file)}
@ -353,7 +364,18 @@ export default function EditorFileTab({
style={{ left: menuPoint.x, top: menuPoint.y }}
/>
</DropdownMenuTrigger>
<DropdownMenuContent className="w-48" sideOffset={0} align="start">
<DropdownMenuContent
className="w-48"
sideOffset={0}
align="start"
onCloseAutoFocus={(event) => {
if (!skipMenuFocusRestoreRef.current) {
return
}
skipMenuFocusRestoreRef.current = false
event.preventDefault()
}}
>
<DropdownMenuItem onSelect={() => onSplitGroup('up', file.tabId ?? file.id)}>
<Rows2 className="mr-1.5 size-3.5" />
Split Up
@ -371,6 +393,18 @@ export default function EditorFileTab({
Split Right
</DropdownMenuItem>
<DropdownMenuSeparator />
<DropdownMenuItem
disabled={!canRename || isRenaming}
onSelect={() => {
skipMenuFocusRestoreRef.current = true
onActivate()
openRenameInput()
}}
>
<Pencil className="mr-1.5 size-3.5" />
Rename
</DropdownMenuItem>
<DropdownMenuSeparator />
<DropdownMenuItem onSelect={onClose}>Close</DropdownMenuItem>
<DropdownMenuItem onSelect={onCloseAll}>Close All Editor Tabs</DropdownMenuItem>
<DropdownMenuItem onSelect={onCloseToRight} disabled={!hasTabsToRight}>

View File

@ -163,4 +163,33 @@ describe('remapOpenEditorTabsForPathChange', () => {
markdownPreviewSourceFileId: floatingNewSourceId
})
})
it('clears the untitled marker when remapping a renamed new markdown file', () => {
const state = useAppStore.getState()
const oldPath = '/repo/untitled.md'
const newPath = '/repo/renamed.md'
state.openFile({
filePath: oldPath,
relativePath: 'untitled.md',
worktreeId: 'wt-1',
language: 'markdown',
isUntitled: true,
mode: 'edit'
})
remapOpenEditorTabsForPathChange({
fromPath: oldPath,
toPath: newPath,
worktreePath: '/repo',
worktreeId: 'wt-1'
})
expect(useAppStore.getState().openFiles).toHaveLength(1)
expect(useAppStore.getState().openFiles[0]).toMatchObject({
filePath: newPath,
relativePath: 'renamed.md'
})
expect(useAppStore.getState().openFiles[0].isUntitled).toBeUndefined()
})
})

View File

@ -160,6 +160,12 @@ export function remapOpenEditorTabsForPathChange({
const draft = state.editorDrafts[file.id]
const wasDirty = file.isDirty
// Why: renameRuntimePath already moved the file. Clear the untitled marker
// before closeFile so its cleanup path does not try to delete the old path.
if (file.isUntitled) {
useAppStore.getState().clearUntitled(file.id)
}
// Why: preview tabs use synthetic ids (`markdown-preview::...`) instead of
// filePath, so close the real tab id before reopening at the new path.
state.closeFile(file.id)