xiaoxue_admin
/
orca
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix: address review findings (#2664)

This commit is contained in:
Jinjing 2026-05-22 22:49:31 -07:00 committed by GitHub
parent 8ad76f7b37
commit 002cab9d82
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
12 changed files with 214 additions and 66 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
src/main/codex-accounts/runtime-home-service.test.ts
View File

@ -42,7 +42,6 @@ function createSettings(overrides: Partial<GlobalSettings> = {}): GlobalSettings
editorAutoSave: false,
editorAutoSaveDelayMs: 1000,
editorMinimapEnabled: false,
markdownReviewToolsEnabled: true,
terminalFontSize: 14,
terminalFontFamily: 'JetBrains Mono',
terminalFontWeight: 500,

1
src/main/codex-accounts/service.test.ts
View File

@ -35,7 +35,6 @@ function createSettings(overrides: Partial<GlobalSettings> = {}): GlobalSettings
editorAutoSave: false,
editorAutoSaveDelayMs: 1000,
editorMinimapEnabled: false,
markdownReviewToolsEnabled: true,
terminalFontSize: 14,
terminalFontFamily: 'JetBrains Mono',
terminalFontWeight: 500,

12
src/renderer/src/components/editor/EditorContent.tsx
View File

@ -106,7 +106,6 @@ export function EditorContent({
isChangesMode,
sideBySide,
showMarkdownTableOfContents = false,
markdownReviewToolsEnabled = true,
onCloseMarkdownTableOfContents = () => {},
pendingEditorReveal,
handleContentChange,
@ -132,7 +131,6 @@ export function EditorContent({
isChangesMode: boolean
sideBySide: boolean
showMarkdownTableOfContents?: boolean
markdownReviewToolsEnabled?: boolean
onCloseMarkdownTableOfContents?: () => void
pendingEditorReveal: PendingEditorReveal | null
handleContentChange: (content: string) => void
@ -289,7 +287,7 @@ export function EditorContent({
onContentChange={handleContentChange}
onSave={isMarkdown ? md.mdSave : handleSave}
worktreeId={activeFile.worktreeId}
markdownAnnotationsEnabled={false}
markdownAnnotationsEnabled={isMarkdown}
conflictDecorationsEnabled={activeFile.conflict?.conflictStatus === 'unresolved'}
revealLine={
matchesPendingEditorReveal(pendingEditorReveal, activeFile)
@ -387,7 +385,7 @@ export function EditorContent({
markdownDocuments={md.markdownDocuments}
showTableOfContents={showMarkdownTableOfContents}
onCloseTableOfContents={onCloseMarkdownTableOfContents}
markdownAnnotationsEnabled={markdownReviewToolsEnabled}
markdownAnnotationsEnabled={true}
markdownAnnotationFilePath={activeFile.relativePath}
markdownSourceLineOffset={fm ? getMarkdownSourceLineOffset(fm.raw) : 0}
markdownReviewContent={currentContent}
@ -427,7 +425,7 @@ export function EditorContent({
scrollCacheKey={`${editorViewStateKey}:preview`}
showTableOfContents={showMarkdownTableOfContents}
onCloseTableOfContents={onCloseMarkdownTableOfContents}
markdownAnnotationsEnabled={false}
markdownAnnotationsEnabled={true}
{...md.previewProps}
/>
</div>
@ -676,7 +674,7 @@ export function EditorContent({
initialAnchor={activeFile.markdownPreviewAnchor ?? null}
showTableOfContents={showMarkdownTableOfContents}
onCloseTableOfContents={onCloseMarkdownTableOfContents}
markdownAnnotationsEnabled={false}
markdownAnnotationsEnabled={true}
{...md.previewProps}
/>
</div>
@ -832,7 +830,7 @@ export function EditorContent({
scrollCacheKey={`${diffViewStateKey}:preview`}
showTableOfContents={showMarkdownTableOfContents}
onCloseTableOfContents={onCloseMarkdownTableOfContents}
markdownAnnotationsEnabled={false}
markdownAnnotationsEnabled={true}
{...md.previewProps}
/>
</div>

2
src/renderer/src/components/editor/EditorPanel.tsx
View File

@ -53,7 +53,6 @@ function EditorPanelInner({
const [showMarkdownTableOfContents, setShowMarkdownTableOfContents] = useState(false)
const [sideBySide, setSideBySide] = useState(settings?.diffDefaultView === 'side-by-side')
const [prevDiffView, setPrevDiffView] = useState(settings?.diffDefaultView)
const markdownReviewToolsEnabled = settings?.markdownReviewToolsEnabled ?? true
if (settings?.diffDefaultView !== prevDiffView) {
setPrevDiffView(settings?.diffDefaultView)
@ -285,7 +284,6 @@ function EditorPanelInner({
model={model}
copiedPathVisible={copiedPathToast?.fileId === activeFile.id}
showMarkdownTableOfContents={showMarkdownTableOfContents}
markdownReviewToolsEnabled={markdownReviewToolsEnabled}
sideBySide={sideBySide}
openFiles={openFiles}
fileContents={fileContents}

3
src/renderer/src/components/editor/EditorPanelShell.tsx
View File

@ -19,7 +19,6 @@ type EditorPanelShellProps = {
model: EditorPanelRenderModel
copiedPathVisible: boolean
showMarkdownTableOfContents: boolean
markdownReviewToolsEnabled: boolean
sideBySide: boolean
openFiles: OpenFile[]
fileContents: Record<string, FileContent>
@ -56,7 +55,6 @@ export function EditorPanelShell({
model,
copiedPathVisible,
showMarkdownTableOfContents,
markdownReviewToolsEnabled,
sideBySide,
openFiles,
fileContents,
@ -144,7 +142,6 @@ export function EditorPanelShell({
handleSaveForFile={onSaveForFile}
reloadFileContent={onReloadFileContent}
showMarkdownTableOfContents={showMarkdownTableOfContents}
markdownReviewToolsEnabled={markdownReviewToolsEnabled}
onCloseMarkdownTableOfContents={onCloseMarkdownTableOfContents}
/>
</Suspense>

86
src/renderer/src/components/editor/MonacoEditor.tsx
View File

@ -35,9 +35,17 @@ import type { DiffComment } from '../../../../shared/types'
import { isMarkdownComment } from '@/lib/diff-comment-compat'
import { useDiffCommentDecorator } from '../diff-comments/useDiffCommentDecorator'
import { DiffCommentPopover } from '../diff-comments/DiffCommentPopover'
import { getDiffCommentPopoverLeft } from '../diff-comments/diff-comment-popover-position'
import {
getDiffCommentPopoverLeft,
getDiffCommentPopoverTop
} from '../diff-comments/diff-comment-popover-position'
import { isLinuxUserAgent } from '../terminal-pane/pane-helpers'
import { installEditorSaveShortcut } from './editor-shortcuts'
import { Plus } from 'lucide-react'
import {
getMonacoMarkdownSelectionAnnotationTarget,
type MonacoMarkdownSelectionAnnotationTarget
} from './monaco-markdown-selection-annotation'
type MonacoEditorProps = {
fileId: string
@ -59,6 +67,10 @@ type MonacoEditorProps = {
autoHeight?: boolean
}
type MarkdownCommentPopoverState = Omit<MonacoMarkdownSelectionAnnotationTarget, 'selectedText'> & {
selectedText?: string
}
export default function MonacoEditor({
fileId,
filePath,
@ -156,12 +168,9 @@ export default function MonacoEditor({
const [gutterMenuOpen, setGutterMenuOpen] = useState(false)
const [gutterMenuPoint, setGutterMenuPoint] = useState({ x: 0, y: 0 })
const [gutterMenuLine, setGutterMenuLine] = useState(1)
const [commentPopover, setCommentPopover] = useState<{
lineNumber: number
startLine?: number
top: number
left?: number
} | null>(null)
const [commentPopover, setCommentPopover] = useState<MarkdownCommentPopoverState | null>(null)
const [selectionAnnotationTarget, setSelectionAnnotationTarget] =
useState<MonacoMarkdownSelectionAnnotationTarget | null>(null)
const isDark =
settings?.theme === 'dark' ||
(settings?.theme === 'system' && window.matchMedia('(prefers-color-scheme: dark)').matches)
@ -199,7 +208,8 @@ export default function MonacoEditor({
filePath: relativePath,
worktreeId: worktreeId ?? '',
comments: shouldShowMarkdownAnnotations ? markdownComments : [],
onAddCommentClick: ({ lineNumber, startLine, top }) =>
onAddCommentClick: ({ lineNumber, startLine, top }) => {
setSelectionAnnotationTarget(null)
setCommentPopover({
lineNumber,
startLine,
@ -207,7 +217,8 @@ export default function MonacoEditor({
left: mountedEditor
? (getDiffCommentPopoverLeft(mountedEditor, editorContainerRef.current) ?? undefined)
: undefined
}),
})
},
onDeleteComment: (id) => {
if (worktreeId) {
void deleteDiffComment(worktreeId, id)
@ -471,11 +482,10 @@ export default function MonacoEditor({
return
}
const update = (): void => {
const top =
mountedEditor.getTopForLineNumber(commentPopover.lineNumber) - mountedEditor.getScrollTop()
const top = getDiffCommentPopoverTop(mountedEditor, commentPopover.lineNumber, undefined)
const left = getDiffCommentPopoverLeft(mountedEditor, editorContainerRef.current)
setCommentPopover((prev) =>
prev ? { ...prev, top, left: left == null ? prev.left : left } : prev
prev ? { ...prev, top: top ?? prev.top, left: left == null ? prev.left : left } : prev
)
}
const scrollSub = mountedEditor.onDidScrollChange(update)
@ -489,6 +499,32 @@ export default function MonacoEditor({
// eslint-disable-next-line react-hooks/exhaustive-deps -- match DiffViewer: don't resubscribe on top updates.
}, [mountedEditor, commentPopover?.lineNumber])
useEffect(() => {
if (!mountedEditor || !shouldShowMarkdownAnnotations || commentPopover) {
setSelectionAnnotationTarget(null)
return
}
const update = (): void => {
const left = getDiffCommentPopoverLeft(mountedEditor, editorContainerRef.current)
setSelectionAnnotationTarget(
getMonacoMarkdownSelectionAnnotationTarget(
mountedEditor,
mountedEditor.getSelection(),
left ?? undefined
)
)
}
update()
const selectionSub = mountedEditor.onDidChangeCursorSelection(update)
const scrollSub = mountedEditor.onDidScrollChange(update)
const layoutSub = mountedEditor.onDidLayoutChange(update)
return () => {
selectionSub.dispose()
scrollSub.dispose()
layoutSub.dispose()
}
}, [commentPopover, mountedEditor, shouldShowMarkdownAnnotations])
const handleSubmitMarkdownComment = async (body: string): Promise<void> => {
if (!commentPopover || !worktreeId) {
return
@ -499,6 +535,7 @@ export default function MonacoEditor({
source: 'markdown',
startLine: commentPopover.startLine,
lineNumber: commentPopover.lineNumber,
selectedText: commentPopover.selectedText,
body,
side: 'modified'
})
@ -666,6 +703,31 @@ export default function MonacoEditor({
onSubmit={handleSubmitMarkdownComment}
/>
)}
{selectionAnnotationTarget && shouldShowMarkdownAnnotations && !commentPopover ? (
<button
type="button"
className="orca-diff-comment-add-btn"
style={{
display: 'flex',
top: Math.max(4, selectionAnnotationTarget.top - 22),
left: selectionAnnotationTarget.left ?? 4
}}
title="Add note on selected text"
aria-label="Add note on selected text"
onMouseDown={(event) => {
event.preventDefault()
event.stopPropagation()
}}
onClick={(event) => {
event.preventDefault()
event.stopPropagation()
setCommentPopover(selectionAnnotationTarget)
setSelectionAnnotationTarget(null)
}}
>
<Plus className="size-3" />
</button>
) : null}
<Editor
height={renderedEditorHeight === null ? '100%' : `${renderedEditorHeight}px`}
language={language}

64
src/renderer/src/components/editor/monaco-markdown-selection-annotation.test.ts Normal file
View File

@ -0,0 +1,64 @@
import { describe, expect, it } from 'vitest'
import type { IRange } from 'monaco-editor'
import { getMonacoMarkdownSelectionAnnotationTarget } from './monaco-markdown-selection-annotation'
function selection(overrides: Partial<IRange> = {}): IRange {
return {
startLineNumber: 2,
startColumn: 3,
endLineNumber: 4,
endColumn: 8,
...overrides
}
}
function editorForSelectedText(selectedText: string, lineCount = 8) {
return {
getModel: () => ({
getLineCount: () => lineCount,
getValueInRange: () => selectedText
}),
getScrollTop: () => 10,
getTopForLineNumber: (lineNumber: number) => lineNumber * 20
}
}
describe('getMonacoMarkdownSelectionAnnotationTarget', () => {
it('maps selected source text to markdown note coordinates', () => {
expect(
getMonacoMarkdownSelectionAnnotationTarget(editorForSelectedText(' chosen '), selection(), 32)
).toEqual({
lineNumber: 4,
startLine: 2,
selectedText: 'chosen',
top: 89,
left: 32
})
})
it('anchors full-line selections to the last selected text line', () => {
expect(
getMonacoMarkdownSelectionAnnotationTarget(
editorForSelectedText('line two\n'),
selection({ startLineNumber: 2, startColumn: 1, endLineNumber: 3, endColumn: 1 })
)
).toEqual({
lineNumber: 2,
selectedText: 'line two',
top: 49,
left: undefined
})
})
it('ignores empty selections and whitespace-only selected text', () => {
expect(
getMonacoMarkdownSelectionAnnotationTarget(
editorForSelectedText('chosen'),
selection({ endLineNumber: 2, endColumn: 3 })
)
).toBeNull()
expect(
getMonacoMarkdownSelectionAnnotationTarget(editorForSelectedText(' '), selection())
).toBeNull()
})
})

71
src/renderer/src/components/editor/monaco-markdown-selection-annotation.ts Normal file
View File

@ -0,0 +1,71 @@
import type { IRange } from 'monaco-editor'
const FALLBACK_LINE_HEIGHT_PX = 19
export type MonacoMarkdownSelectionAnnotationTarget = {
lineNumber: number
startLine?: number
selectedText: string
top: number
left?: number
}
type MonacoMarkdownSelectionModel = {
getLineCount: () => number
getValueInRange: (range: IRange) => string
}
type MonacoMarkdownSelectionEditor = {
getModel: () => MonacoMarkdownSelectionModel | null
getScrollTop: () => number
getTopForLineNumber: (lineNumber: number) => number
}
function isEmptySelection(selection: IRange): boolean {
return (
selection.startLineNumber === selection.endLineNumber &&
selection.startColumn === selection.endColumn
)
}
function getSelectionTextEndLine(selection: IRange): number {
if (selection.endColumn === 1 && selection.endLineNumber > selection.startLineNumber) {
return selection.endLineNumber - 1
}
return selection.endLineNumber
}
export function getMonacoMarkdownSelectionAnnotationTarget(
editorInstance: MonacoMarkdownSelectionEditor,
selection: IRange | null,
left?: number
): MonacoMarkdownSelectionAnnotationTarget | null {
if (!selection || isEmptySelection(selection)) {
return null
}
const model = editorInstance.getModel()
if (!model) {
return null
}
const selectedText = model.getValueInRange(selection).trim()
if (!selectedText) {
return null
}
const textEndLine = getSelectionTextEndLine(selection)
const startLine = Math.min(selection.startLineNumber, textEndLine)
const lineNumber = Math.max(selection.startLineNumber, textEndLine)
if (startLine < 1 || lineNumber > model.getLineCount()) {
return null
}
const top =
editorInstance.getTopForLineNumber(lineNumber) -
editorInstance.getScrollTop() +
FALLBACK_LINE_HEIGHT_PX
return {
lineNumber,
startLine: startLine === lineNumber ? undefined : startLine,
selectedText,
top,
left
}
}

32
src/renderer/src/components/settings/GeneralPane.tsx
View File

@ -625,38 +625,6 @@ export function GeneralPane({ settings, updateSettings }: GeneralPaneProps): Rea
/>
</button>
</SearchableSetting>
<SearchableSetting
title="Markdown Review Notes"
description="Show local markdown review note controls in rich editor mode."
keywords={['markdown', 'review', 'notes', 'annotations', 'agents']}
className="flex items-center justify-between gap-4 px-1 py-2"
>
<div className="space-y-0.5">
<Label>Markdown Review Notes</Label>
<p className="text-xs text-muted-foreground">
Show local markdown note controls in rich editor mode and agent handoff actions.
</p>
</div>
<button
role="switch"
aria-checked={settings.markdownReviewToolsEnabled}
onClick={() =>
updateSettings({
markdownReviewToolsEnabled: !settings.markdownReviewToolsEnabled
})
}
className={`relative inline-flex h-5 w-9 shrink-0 cursor-pointer items-center rounded-full border border-transparent transition-colors ${
settings.markdownReviewToolsEnabled ? 'bg-foreground' : 'bg-muted-foreground/30'
}`}
>
<span
className={`pointer-events-none block size-3.5 rounded-full bg-background shadow-sm transition-transform ${
settings.markdownReviewToolsEnabled ? 'translate-x-4' : 'translate-x-0.5'
}`}
/>
</button>
</SearchableSetting>
</section>
) : null,
matchesSettingsSearch(searchQuery, GENERAL_CLI_SEARCH_ENTRIES) ? (

5
src/renderer/src/components/settings/general-search.ts
View File

@ -53,11 +53,6 @@ export const GENERAL_EDITOR_SEARCH_ENTRIES: SettingsSearchEntry[] = [
title: 'Minimap',
description: 'Show the minimap overview when editing a file.',
keywords: ['minimap', 'overview', 'code', 'scroll']
},
{
title: 'Markdown Review Notes',
description: 'Show local markdown review note controls in rich editor mode.',
keywords: ['markdown', 'review', 'notes', 'annotations', 'agents']
}
]

1
src/shared/constants.ts
View File

@ -160,7 +160,6 @@ export function getDefaultSettings(homedir: string): GlobalSettings {
editorAutoSave: false,
editorAutoSaveDelayMs: DEFAULT_EDITOR_AUTO_SAVE_DELAY_MS,
editorMinimapEnabled: false,
markdownReviewToolsEnabled: true,
primarySelectionMiddleClickPaste: getDefaultPrimarySelectionMiddleClickPaste(),
primarySelectionMiddleClickPasteDefaultedForLinux:
typeof process !== 'undefined' && process.platform === 'linux',

2
src/shared/types.ts
View File

@ -1527,8 +1527,6 @@ export type GlobalSettings = {
editorAutoSave: boolean
editorAutoSaveDelayMs: number
editorMinimapEnabled: boolean
/** Whether local markdown review note controls and the review panel are shown. */
markdownReviewToolsEnabled: boolean
/** Why: mirrors terminal selection-paste muscle memory without mutating the
* normal system clipboard; Linux and macOS enable it by default, Windows
* leaves middle-click semantics unchanged unless the user opts in. */