From 002cab9d826cefa50210d9d29af7ed41d93cf6a8 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Fri, 22 May 2026 22:49:31 -0700 Subject: [PATCH] fix: address review findings (#2664) --- .../runtime-home-service.test.ts | 1 - src/main/codex-accounts/service.test.ts | 1 - .../src/components/editor/EditorContent.tsx | 12 ++- .../src/components/editor/EditorPanel.tsx | 2 - .../components/editor/EditorPanelShell.tsx | 3 - .../src/components/editor/MonacoEditor.tsx | 86 ++++++++++++++++--- ...naco-markdown-selection-annotation.test.ts | 64 ++++++++++++++ .../monaco-markdown-selection-annotation.ts | 71 +++++++++++++++ .../src/components/settings/GeneralPane.tsx | 32 ------- .../src/components/settings/general-search.ts | 5 -- src/shared/constants.ts | 1 - src/shared/types.ts | 2 - 12 files changed, 214 insertions(+), 66 deletions(-) create mode 100644 src/renderer/src/components/editor/monaco-markdown-selection-annotation.test.ts create mode 100644 src/renderer/src/components/editor/monaco-markdown-selection-annotation.ts diff --git a/src/main/codex-accounts/runtime-home-service.test.ts b/src/main/codex-accounts/runtime-home-service.test.ts index 3e87e753e..cd31d8f65 100644 --- a/src/main/codex-accounts/runtime-home-service.test.ts +++ b/src/main/codex-accounts/runtime-home-service.test.ts @@ -42,7 +42,6 @@ function createSettings(overrides: Partial = {}): GlobalSettings editorAutoSave: false, editorAutoSaveDelayMs: 1000, editorMinimapEnabled: false, - markdownReviewToolsEnabled: true, terminalFontSize: 14, terminalFontFamily: 'JetBrains Mono', terminalFontWeight: 500, diff --git a/src/main/codex-accounts/service.test.ts b/src/main/codex-accounts/service.test.ts index 7f9ea5c52..c1c566257 100644 --- a/src/main/codex-accounts/service.test.ts +++ b/src/main/codex-accounts/service.test.ts @@ -35,7 +35,6 @@ function createSettings(overrides: Partial = {}): GlobalSettings editorAutoSave: false, editorAutoSaveDelayMs: 1000, editorMinimapEnabled: false, - markdownReviewToolsEnabled: true, terminalFontSize: 14, terminalFontFamily: 'JetBrains Mono', terminalFontWeight: 500, diff --git a/src/renderer/src/components/editor/EditorContent.tsx b/src/renderer/src/components/editor/EditorContent.tsx index 56059afc0..d3806fc18 100644 --- a/src/renderer/src/components/editor/EditorContent.tsx +++ b/src/renderer/src/components/editor/EditorContent.tsx @@ -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} /> @@ -676,7 +674,7 @@ export function EditorContent({ initialAnchor={activeFile.markdownPreviewAnchor ?? null} showTableOfContents={showMarkdownTableOfContents} onCloseTableOfContents={onCloseMarkdownTableOfContents} - markdownAnnotationsEnabled={false} + markdownAnnotationsEnabled={true} {...md.previewProps} /> @@ -832,7 +830,7 @@ export function EditorContent({ scrollCacheKey={`${diffViewStateKey}:preview`} showTableOfContents={showMarkdownTableOfContents} onCloseTableOfContents={onCloseMarkdownTableOfContents} - markdownAnnotationsEnabled={false} + markdownAnnotationsEnabled={true} {...md.previewProps} /> diff --git a/src/renderer/src/components/editor/EditorPanel.tsx b/src/renderer/src/components/editor/EditorPanel.tsx index ad1e517db..63d85e016 100644 --- a/src/renderer/src/components/editor/EditorPanel.tsx +++ b/src/renderer/src/components/editor/EditorPanel.tsx @@ -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} diff --git a/src/renderer/src/components/editor/EditorPanelShell.tsx b/src/renderer/src/components/editor/EditorPanelShell.tsx index 9d27c81fe..f0b7b9a9b 100644 --- a/src/renderer/src/components/editor/EditorPanelShell.tsx +++ b/src/renderer/src/components/editor/EditorPanelShell.tsx @@ -19,7 +19,6 @@ type EditorPanelShellProps = { model: EditorPanelRenderModel copiedPathVisible: boolean showMarkdownTableOfContents: boolean - markdownReviewToolsEnabled: boolean sideBySide: boolean openFiles: OpenFile[] fileContents: Record @@ -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} /> diff --git a/src/renderer/src/components/editor/MonacoEditor.tsx b/src/renderer/src/components/editor/MonacoEditor.tsx index 9885dd7fc..f9006b66d 100644 --- a/src/renderer/src/components/editor/MonacoEditor.tsx +++ b/src/renderer/src/components/editor/MonacoEditor.tsx @@ -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 & { + 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(null) + const [selectionAnnotationTarget, setSelectionAnnotationTarget] = + useState(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 => { 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 ? ( + + ) : null} = {}): 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() + }) +}) diff --git a/src/renderer/src/components/editor/monaco-markdown-selection-annotation.ts b/src/renderer/src/components/editor/monaco-markdown-selection-annotation.ts new file mode 100644 index 000000000..178787328 --- /dev/null +++ b/src/renderer/src/components/editor/monaco-markdown-selection-annotation.ts @@ -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 + } +} diff --git a/src/renderer/src/components/settings/GeneralPane.tsx b/src/renderer/src/components/settings/GeneralPane.tsx index d10883828..522cdc821 100644 --- a/src/renderer/src/components/settings/GeneralPane.tsx +++ b/src/renderer/src/components/settings/GeneralPane.tsx @@ -625,38 +625,6 @@ export function GeneralPane({ settings, updateSettings }: GeneralPaneProps): Rea /> - - -
- -

- Show local markdown note controls in rich editor mode and agent handoff actions. -

-
- - ) : null, matchesSettingsSearch(searchQuery, GENERAL_CLI_SEARCH_ENTRIES) ? ( diff --git a/src/renderer/src/components/settings/general-search.ts b/src/renderer/src/components/settings/general-search.ts index f1fdfc259..492fc38f0 100644 --- a/src/renderer/src/components/settings/general-search.ts +++ b/src/renderer/src/components/settings/general-search.ts @@ -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'] } ] diff --git a/src/shared/constants.ts b/src/shared/constants.ts index 9adcf6836..a55248766 100644 --- a/src/shared/constants.ts +++ b/src/shared/constants.ts @@ -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', diff --git a/src/shared/types.ts b/src/shared/types.ts index d383ce028..6fce09819 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -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. */