Add review note chord guards (#9412)
* Guard add-review-note chord to prevent remount (product B) - Scoped guards consume chord at composer level to prevent remount and leakage - Handle OS key-repeat: ignore when no draft, consume when one is mounted - Clear stale block keys in markdown preview when content renumbers - Flush pending selection before reading targets to fix timing races * fix(editor): repair add-review-note guard tests and close remaining chord gaps - Update the product-B guard tests to the Mod+Shift+A default binding (#9257 retired Mod+Alt+N as AltGr-unsafe); they asserted the old chord, so seven landed red and the rich-editor repeat test passed vacuously. - Monaco: recompute the annotation target from the live selection at keydown instead of the render-lagged ref, so a chord right after a drag cannot open on the previous selection or miss a fresh one. - Preview: key the stale-block-key cleanup on renderedContent (the DOM the block keys live in), which can lag content during external-edit section preservation. Co-authored-by: Orca <help@stably.ai> * fix(editor): mirror shortcut-guard refs in effects instead of render body CodeRabbit on #9412: render-body ref writes can leak from a render pass React replays and discards. Move the state->ref mirrors for commentPopoverRef / shouldShowMarkdownAnnotationsRef (MonacoEditor) and activeAnnotationBlockKeyRef (MarkdownPreview) into effects; same-tick keydown paths keep their eager event-handler writes. Co-authored-by: Orca <help@stably.ai> --------- Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Jinjing
GitHub
parent
02ff7c7465
commit
f7c2c1276c
|
|
@ -8,6 +8,7 @@ import {
|
|||
hasBoundedCommentBodyText
|
||||
} from '@/lib/comment-body-submit-state'
|
||||
import { translate } from '@/i18n/i18n'
|
||||
import { installOpenDraftAddReviewNoteGuard } from '../editor/editor-shortcuts'
|
||||
import { resolveDiffCommentPopoverTop } from './diff-comment-popover-position'
|
||||
|
||||
// Why: rendered as a DOM sibling overlay inside the editor container rather
|
||||
|
|
@ -141,6 +142,17 @@ export function DiffCommentPopover({
|
|||
textarea?.focus()
|
||||
}, [])
|
||||
|
||||
// Why: product B — the composer autofocuses its textarea, so a second
|
||||
// add-review-note chord must not remount the draft; consume it on the
|
||||
// popover subtree (not window) so other panes/surfaces keep their chord.
|
||||
useEffect(() => {
|
||||
const popover = popoverRef.current
|
||||
if (!popover) {
|
||||
return
|
||||
}
|
||||
return installOpenDraftAddReviewNoteGuard(popover)
|
||||
}, [])
|
||||
|
||||
// Why: Monaco's editor area does not bubble a synthetic React click up to
|
||||
// the popover's onClick. Without a document-level mousedown listener, the
|
||||
// popover has no way to detect clicks outside its own bounds. We keep the
|
||||
|
|
|
|||
|
|
@ -69,9 +69,10 @@ import {
|
|||
setActiveMarkdownPreviewSearchMatch
|
||||
} from './markdown-preview-search'
|
||||
import {
|
||||
getMarkdownAnnotationBlockKeyForSelection,
|
||||
isMarkdownPreviewAddReviewNoteShortcut
|
||||
previewHasAnnotationBlockKey,
|
||||
resolveMarkdownPreviewAddReviewNoteKey
|
||||
} from './markdown-preview-annotation-shortcut'
|
||||
import { installOpenDraftAddReviewNoteGuard } from './editor-shortcuts'
|
||||
import { usePreserveSectionDuringExternalEdit } from './usePreserveSectionDuringExternalEdit'
|
||||
import { openHttpLink, type HttpLinkSourceOwner } from '@/lib/http-link-routing'
|
||||
import { getShortcutPlatform } from '@/lib/shortcut-platform'
|
||||
|
|
@ -636,6 +637,28 @@ export default function MarkdownPreview({
|
|||
? (frontmatterVisibleByFile[toggleableSourceFileId] ?? true)
|
||||
: true
|
||||
const [activeAnnotationBlockKey, setActiveAnnotationBlockKey] = useState<string | null>(null)
|
||||
const activeAnnotationBlockKeyRef = useRef(activeAnnotationBlockKey)
|
||||
// Why: mirrored in an effect (not the render body) so a discarded render
|
||||
// pass cannot leak into the ref; keydown paths still write it eagerly.
|
||||
useEffect(() => {
|
||||
activeAnnotationBlockKeyRef.current = activeAnnotationBlockKey
|
||||
}, [activeAnnotationBlockKey])
|
||||
// Why: line-derived block keys can go stale after content renumbers; drop them
|
||||
// when the block no longer mounts so the shortcut cannot lock out forever.
|
||||
useEffect(() => {
|
||||
if (!activeAnnotationBlockKey) {
|
||||
return
|
||||
}
|
||||
const root = rootRef.current
|
||||
if (!root || previewHasAnnotationBlockKey(root, activeAnnotationBlockKey)) {
|
||||
return
|
||||
}
|
||||
// Why: the mirror effect above re-syncs the ref once this setState commits;
|
||||
// only the same-tick keydown paths need an eager manual write.
|
||||
setActiveAnnotationBlockKey(null)
|
||||
// Why: keyed on renderedContent (not content) — the DOM the block keys live
|
||||
// in derives from it and can lag content during external-edit preservation.
|
||||
}, [activeAnnotationBlockKey, renderedContent])
|
||||
const [reviewNotesCopied, setReviewNotesCopied] = useState(false)
|
||||
const [copiedReviewNoteId, setCopiedReviewNoteId] = useState<string | null>(null)
|
||||
const reviewNotesCopiedResetTimerRef = useRef<number | null>(null)
|
||||
|
|
@ -920,17 +943,33 @@ export default function MarkdownPreview({
|
|||
return
|
||||
}
|
||||
|
||||
if (
|
||||
isMarkdownPreviewAddReviewNoteShortcut(event, getShortcutPlatform(), keybindings) &&
|
||||
targetInsidePreview &&
|
||||
markdownAnnotationsEnabled
|
||||
) {
|
||||
const blockKey = getMarkdownAnnotationBlockKeyForSelection(root, window.getSelection())
|
||||
if (blockKey) {
|
||||
event.preventDefault()
|
||||
event.stopPropagation()
|
||||
setActiveAnnotationBlockKey(blockKey)
|
||||
}
|
||||
const reviewNoteKey = resolveMarkdownPreviewAddReviewNoteKey({
|
||||
event,
|
||||
platform: getShortcutPlatform(),
|
||||
keybindings,
|
||||
targetInsidePreview,
|
||||
markdownAnnotationsEnabled,
|
||||
activeAnnotationBlockKey: activeAnnotationBlockKeyRef.current,
|
||||
root,
|
||||
selection: window.getSelection()
|
||||
})
|
||||
if (reviewNoteKey.action === 'consume') {
|
||||
event.preventDefault()
|
||||
event.stopPropagation()
|
||||
return
|
||||
}
|
||||
if (reviewNoteKey.action === 'clear-stale-and-ignore') {
|
||||
// Why: drop a line-derived key that no longer mounts a composer so the
|
||||
// shortcut cannot stay permanently consumed after content renumbers.
|
||||
activeAnnotationBlockKeyRef.current = null
|
||||
setActiveAnnotationBlockKey(null)
|
||||
return
|
||||
}
|
||||
if (reviewNoteKey.action === 'open') {
|
||||
event.preventDefault()
|
||||
event.stopPropagation()
|
||||
activeAnnotationBlockKeyRef.current = reviewNoteKey.blockKey
|
||||
setActiveAnnotationBlockKey(reviewNoteKey.blockKey)
|
||||
return
|
||||
}
|
||||
|
||||
|
|
@ -2052,6 +2091,18 @@ function MarkdownAnnotationComposer({
|
|||
const [body, setBody] = useState('')
|
||||
const [submitting, setSubmitting] = useState(false)
|
||||
const mountedRef = useMountedRef()
|
||||
const composerRef = useRef<HTMLDivElement | null>(null)
|
||||
|
||||
// Why: product B — composer focus is in the textarea, so consume the bindable
|
||||
// add-review-note chord on the composer subtree (same guard as
|
||||
// DiffCommentPopover) rather than window, so other surfaces keep their chord.
|
||||
useEffect(() => {
|
||||
const composer = composerRef.current
|
||||
if (!composer) {
|
||||
return
|
||||
}
|
||||
return installOpenDraftAddReviewNoteGuard(composer)
|
||||
}, [])
|
||||
|
||||
const focusTextareaRef = useCallback((textarea: HTMLTextAreaElement | null): void => {
|
||||
// Why: opening an annotation composer should focus the draft field on the
|
||||
|
|
@ -2082,7 +2133,11 @@ function MarkdownAnnotationComposer({
|
|||
}
|
||||
|
||||
return (
|
||||
<div className="markdown-annotation-composer" onClick={(event) => event.stopPropagation()}>
|
||||
<div
|
||||
ref={composerRef}
|
||||
className="markdown-annotation-composer"
|
||||
onClick={(event) => event.stopPropagation()}
|
||||
>
|
||||
<div className="orca-diff-comment-popover-label">
|
||||
{translate('auto.components.editor.MarkdownPreview.b1bfc04034', 'Selected text')}
|
||||
</div>
|
||||
|
|
|
|||
|
|
@ -202,10 +202,13 @@ export default function MonacoEditor({
|
|||
const [commentPopover, setCommentPopover] = useState<MarkdownCommentPopoverState | null>(null)
|
||||
const [selectionAnnotationTarget, setSelectionAnnotationTarget] =
|
||||
useState<MonacoMarkdownSelectionAnnotationTarget | null>(null)
|
||||
// Why: the Monaco mount closure installs its keydown listeners once, so the
|
||||
// add-review-note shortcut reads the live selection target through a ref.
|
||||
const selectionAnnotationTargetRef = useRef<MonacoMarkdownSelectionAnnotationTarget | null>(null)
|
||||
selectionAnnotationTargetRef.current = selectionAnnotationTarget
|
||||
// Why: claim open drafts synchronously so a same-tick second chord cannot
|
||||
// remount the composer before React commits commentPopover state. Mirrored
|
||||
// in an effect so a discarded render pass cannot leak into the ref.
|
||||
const commentPopoverRef = useRef<MarkdownCommentPopoverState | null>(null)
|
||||
useEffect(() => {
|
||||
commentPopoverRef.current = commentPopover
|
||||
}, [commentPopover])
|
||||
const isDark =
|
||||
settings?.theme === 'dark' ||
|
||||
(settings?.theme === 'system' && window.matchMedia('(prefers-color-scheme: dark)').matches)
|
||||
|
|
@ -228,6 +231,12 @@ export default function MonacoEditor({
|
|||
|
||||
const shouldShowMarkdownAnnotations =
|
||||
markdownAnnotationsEnabled && language === 'markdown' && Boolean(worktreeId)
|
||||
// Why: the Monaco mount closure installs its keydown listeners once, so the
|
||||
// add-review-note shortcut reads the current enablement through a ref.
|
||||
const shouldShowMarkdownAnnotationsRef = useRef(shouldShowMarkdownAnnotations)
|
||||
useEffect(() => {
|
||||
shouldShowMarkdownAnnotationsRef.current = shouldShowMarkdownAnnotations
|
||||
}, [shouldShowMarkdownAnnotations])
|
||||
|
||||
const pendingScrollForThisEditor = useMemo(() => {
|
||||
if (!shouldShowMarkdownAnnotations || !scrollToDiffCommentId) {
|
||||
|
|
@ -421,14 +430,28 @@ export default function MonacoEditor({
|
|||
propsRef.current.onSave(value)
|
||||
})
|
||||
const cleanupFindShortcut = installMonacoEditorFindShortcut(editorInstance)
|
||||
// Opens the same composer as the selection "+" button; the target ref
|
||||
// mirrors the last-rendered selection target and is null unless markdown
|
||||
// annotations are enabled and text is selected.
|
||||
// Opens the same composer as the selection "+" button.
|
||||
const cleanupAddReviewNoteShortcut = installEditorAddReviewNoteShortcut(editorDomNode, () => {
|
||||
const target = selectionAnnotationTargetRef.current
|
||||
// Why: product B — keep an open draft instead of remounting (same-tick
|
||||
// races and editor-focused second chords before the composer guard runs).
|
||||
if (commentPopoverRef.current) {
|
||||
return true
|
||||
}
|
||||
if (!shouldShowMarkdownAnnotationsRef.current) {
|
||||
return false
|
||||
}
|
||||
// Why: the rendered target ref lags onDidChangeCursorSelection by a
|
||||
// render, so a chord right after a drag could open on the previous
|
||||
// selection (or miss a fresh one); read Monaco's live selection instead.
|
||||
const target = getMonacoMarkdownSelectionAnnotationTarget(
|
||||
editorInstance,
|
||||
editorInstance.getSelection(),
|
||||
getDiffCommentPopoverLeft(editorInstance, editorContainerRef.current) ?? undefined
|
||||
)
|
||||
if (!target) {
|
||||
return false
|
||||
}
|
||||
commentPopoverRef.current = target
|
||||
setCommentPopover(target)
|
||||
setSelectionAnnotationTarget(null)
|
||||
return true
|
||||
|
|
|
|||
|
|
@ -21,7 +21,8 @@ import {
|
|||
installEditorAddReviewNoteShortcut,
|
||||
installEditorFindShortcut,
|
||||
installMonacoDiffChangeNavigationShortcut,
|
||||
installMonacoEditorFindShortcut
|
||||
installMonacoEditorFindShortcut,
|
||||
installOpenDraftAddReviewNoteGuard
|
||||
} from './editor-shortcuts'
|
||||
|
||||
type ShortcutFixture = {
|
||||
|
|
@ -270,6 +271,50 @@ describe('installEditorAddReviewNoteShortcut', () => {
|
|||
})
|
||||
})
|
||||
|
||||
describe('installOpenDraftAddReviewNoteGuard', () => {
|
||||
it('consumes the add-review-note chord including OS key-repeat (product B)', () => {
|
||||
// Why: the guard is scoped to the composer subtree, so mirror that with a
|
||||
// container wrapping the focused textarea rather than attaching to window.
|
||||
const container = document.createElement('div')
|
||||
const input = document.createElement('textarea')
|
||||
const onDownstreamKeyDown = vi.fn()
|
||||
container.appendChild(input)
|
||||
document.body.appendChild(container)
|
||||
input.addEventListener('keydown', onDownstreamKeyDown)
|
||||
const dispose = installOpenDraftAddReviewNoteGuard(container)
|
||||
|
||||
const first = dispatchKeyDown(input, {
|
||||
key: 'a',
|
||||
code: 'KeyA',
|
||||
metaKey: true,
|
||||
shiftKey: true
|
||||
})
|
||||
const repeat = dispatchKeyDown(input, {
|
||||
key: 'a',
|
||||
code: 'KeyA',
|
||||
metaKey: true,
|
||||
shiftKey: true,
|
||||
repeat: true
|
||||
})
|
||||
const unrelated = dispatchKeyDown(input, { key: 'a', code: 'KeyA', metaKey: true })
|
||||
|
||||
expect(first.defaultPrevented).toBe(true)
|
||||
expect(repeat.defaultPrevented).toBe(true)
|
||||
expect(unrelated.defaultPrevented).toBe(false)
|
||||
// Capture-phase guard stops propagation before the target listener.
|
||||
expect(onDownstreamKeyDown).toHaveBeenCalledTimes(1)
|
||||
|
||||
dispose()
|
||||
const afterDispose = dispatchKeyDown(input, {
|
||||
key: 'a',
|
||||
code: 'KeyA',
|
||||
metaKey: true,
|
||||
shiftKey: true
|
||||
})
|
||||
expect(afterDispose.defaultPrevented).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
describe('installMonacoDiffChangeNavigationShortcut', () => {
|
||||
function createDiffNavigationFixture(): {
|
||||
container: HTMLDivElement
|
||||
|
|
|
|||
|
|
@ -84,7 +84,12 @@ export function installEditorAddReviewNoteShortcut(
|
|||
onAddReviewNote: () => boolean
|
||||
): () => void {
|
||||
const handleKeyDown = (event: KeyboardEvent): void => {
|
||||
if (event.repeat || !editorShortcutMatches('editor.addReviewNote', event)) {
|
||||
if (!editorShortcutMatches('editor.addReviewNote', event)) {
|
||||
return
|
||||
}
|
||||
// Why: ignore OS key-repeat so a held chord cannot thrash open/remount.
|
||||
// Open drafts are consumed by installOpenDraftAddReviewNoteGuard instead.
|
||||
if (event.repeat) {
|
||||
return
|
||||
}
|
||||
// Why: only consume the chord when a composer actually opens; on files
|
||||
|
|
@ -100,6 +105,28 @@ export function installEditorAddReviewNoteShortcut(
|
|||
return () => target.removeEventListener('keydown', handleKeyDown, true)
|
||||
}
|
||||
|
||||
/**
|
||||
* While a review-note/diff-comment draft composer is mounted, consume the
|
||||
* bindable add-review-note chord (including OS key-repeat) so a second press
|
||||
* cannot remount the composer or leak into other handlers (product B).
|
||||
*
|
||||
* Scoped to the composer's own subtree (which contains the focused textarea)
|
||||
* rather than `window` so a draft open in one editor pane never swallows the
|
||||
* chord typed into a different pane or surface.
|
||||
*/
|
||||
export function installOpenDraftAddReviewNoteGuard(target: HTMLElement): () => void {
|
||||
const handleKeyDown = (event: KeyboardEvent): void => {
|
||||
if (!editorShortcutMatches('editor.addReviewNote', event)) {
|
||||
return
|
||||
}
|
||||
event.preventDefault()
|
||||
event.stopPropagation()
|
||||
}
|
||||
|
||||
target.addEventListener('keydown', handleKeyDown, true)
|
||||
return () => target.removeEventListener('keydown', handleKeyDown, true)
|
||||
}
|
||||
|
||||
type MonacoFindShortcutEditor = {
|
||||
getAction: (id: string) => { run: () => void | Promise<void> } | null
|
||||
getContainerDomNode: () => HTMLElement
|
||||
|
|
|
|||
|
|
@ -3,7 +3,9 @@
|
|||
import { afterEach, describe, expect, it } from 'vitest'
|
||||
import {
|
||||
getMarkdownAnnotationBlockKeyForSelection,
|
||||
isMarkdownPreviewAddReviewNoteShortcut
|
||||
isMarkdownPreviewAddReviewNoteShortcut,
|
||||
previewHasAnnotationBlockKey,
|
||||
resolveMarkdownPreviewAddReviewNoteKey
|
||||
} from './markdown-preview-annotation-shortcut'
|
||||
|
||||
function createPreviewFixture(): {
|
||||
|
|
@ -105,3 +107,115 @@ describe('isMarkdownPreviewAddReviewNoteShortcut', () => {
|
|||
).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
describe('resolveMarkdownPreviewAddReviewNoteKey', () => {
|
||||
const chord = {
|
||||
key: 'a',
|
||||
code: 'KeyA',
|
||||
metaKey: true,
|
||||
ctrlKey: false,
|
||||
altKey: false,
|
||||
shiftKey: true,
|
||||
repeat: false
|
||||
}
|
||||
|
||||
it('consumes the chord while a mounted draft block is open (product B)', () => {
|
||||
const { root } = createPreviewFixture()
|
||||
|
||||
expect(
|
||||
resolveMarkdownPreviewAddReviewNoteKey({
|
||||
event: chord,
|
||||
platform: 'darwin',
|
||||
targetInsidePreview: true,
|
||||
markdownAnnotationsEnabled: true,
|
||||
activeAnnotationBlockKey: 'p:3-5',
|
||||
root,
|
||||
selection: null
|
||||
})
|
||||
).toEqual({ action: 'consume' })
|
||||
})
|
||||
|
||||
it('consumes OS key-repeat while a mounted draft is open', () => {
|
||||
const { root } = createPreviewFixture()
|
||||
|
||||
expect(
|
||||
resolveMarkdownPreviewAddReviewNoteKey({
|
||||
event: { ...chord, repeat: true },
|
||||
platform: 'darwin',
|
||||
targetInsidePreview: true,
|
||||
markdownAnnotationsEnabled: true,
|
||||
activeAnnotationBlockKey: 'p:3-5',
|
||||
root,
|
||||
selection: null
|
||||
})
|
||||
).toEqual({ action: 'consume' })
|
||||
})
|
||||
|
||||
it('ignores OS key-repeat when no draft is open', () => {
|
||||
const { root, paragraph } = createPreviewFixture()
|
||||
const selection = selectTextIn(paragraph)
|
||||
|
||||
expect(
|
||||
resolveMarkdownPreviewAddReviewNoteKey({
|
||||
event: { ...chord, repeat: true },
|
||||
platform: 'darwin',
|
||||
targetInsidePreview: true,
|
||||
markdownAnnotationsEnabled: true,
|
||||
activeAnnotationBlockKey: null,
|
||||
root,
|
||||
selection
|
||||
})
|
||||
).toEqual({ action: 'ignore' })
|
||||
})
|
||||
|
||||
it('opens the composer for a live selection when no draft is open', () => {
|
||||
const { root, paragraph } = createPreviewFixture()
|
||||
const selection = selectTextIn(paragraph)
|
||||
|
||||
expect(
|
||||
resolveMarkdownPreviewAddReviewNoteKey({
|
||||
event: chord,
|
||||
platform: 'darwin',
|
||||
targetInsidePreview: true,
|
||||
markdownAnnotationsEnabled: true,
|
||||
activeAnnotationBlockKey: null,
|
||||
root,
|
||||
selection
|
||||
})
|
||||
).toEqual({ action: 'open', blockKey: 'p:3-5' })
|
||||
})
|
||||
|
||||
it('clears a stale block key that no longer mounts a composer', () => {
|
||||
const { root, paragraph } = createPreviewFixture()
|
||||
const selection = selectTextIn(paragraph)
|
||||
|
||||
expect(previewHasAnnotationBlockKey(root, 'p:9-9')).toBe(false)
|
||||
expect(
|
||||
resolveMarkdownPreviewAddReviewNoteKey({
|
||||
event: chord,
|
||||
platform: 'darwin',
|
||||
targetInsidePreview: true,
|
||||
markdownAnnotationsEnabled: true,
|
||||
activeAnnotationBlockKey: 'p:9-9',
|
||||
root,
|
||||
selection
|
||||
})
|
||||
).toEqual({ action: 'open', blockKey: 'p:3-5' })
|
||||
})
|
||||
|
||||
it('clears a stale key on repeat without opening', () => {
|
||||
const { root } = createPreviewFixture()
|
||||
|
||||
expect(
|
||||
resolveMarkdownPreviewAddReviewNoteKey({
|
||||
event: { ...chord, repeat: true },
|
||||
platform: 'darwin',
|
||||
targetInsidePreview: true,
|
||||
markdownAnnotationsEnabled: true,
|
||||
activeAnnotationBlockKey: 'p:9-9',
|
||||
root,
|
||||
selection: null
|
||||
})
|
||||
).toEqual({ action: 'clear-stale-and-ignore' })
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -36,3 +36,86 @@ export function getMarkdownAnnotationBlockKeyForSelection(
|
|||
closestAnnotationBlockKey(selection.focusNode, root)
|
||||
)
|
||||
}
|
||||
|
||||
export function previewHasAnnotationBlockKey(root: HTMLElement, blockKey: string): boolean {
|
||||
// Why: walk attributes instead of building a CSS selector so keys never need
|
||||
// CSS.escape (unavailable in some test environments).
|
||||
for (const block of root.querySelectorAll('[data-annotation-block-key]')) {
|
||||
if (block.getAttribute('data-annotation-block-key') === blockKey) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
export type MarkdownPreviewAddReviewNoteKeyResult =
|
||||
| { action: 'ignore' }
|
||||
| { action: 'consume' }
|
||||
| { action: 'open'; blockKey: string }
|
||||
| { action: 'clear-stale-and-ignore' }
|
||||
|
||||
/**
|
||||
* Pure decision for the preview add-review-note chord. Keeps product B, OS
|
||||
* key-repeat, and stale-block-key handling out of the React component body.
|
||||
*/
|
||||
export function resolveMarkdownPreviewAddReviewNoteKey(options: {
|
||||
event: Pick<
|
||||
KeyboardEvent,
|
||||
'key' | 'code' | 'metaKey' | 'ctrlKey' | 'altKey' | 'shiftKey' | 'repeat'
|
||||
>
|
||||
platform: NodeJS.Platform
|
||||
keybindings?: KeybindingOverrides
|
||||
targetInsidePreview: boolean
|
||||
markdownAnnotationsEnabled: boolean
|
||||
activeAnnotationBlockKey: string | null
|
||||
root: HTMLElement
|
||||
selection: Selection | null
|
||||
}): MarkdownPreviewAddReviewNoteKeyResult {
|
||||
const {
|
||||
event,
|
||||
platform,
|
||||
keybindings,
|
||||
targetInsidePreview,
|
||||
markdownAnnotationsEnabled,
|
||||
activeAnnotationBlockKey,
|
||||
root,
|
||||
selection
|
||||
} = options
|
||||
|
||||
if (
|
||||
!isMarkdownPreviewAddReviewNoteShortcut(event, platform, keybindings) ||
|
||||
!targetInsidePreview ||
|
||||
!markdownAnnotationsEnabled
|
||||
) {
|
||||
return { action: 'ignore' }
|
||||
}
|
||||
|
||||
if (activeAnnotationBlockKey) {
|
||||
// Why: only treat the key as an open draft when the block still mounts a
|
||||
// composer; a stale key after content renumber must not lock the shortcut.
|
||||
if (previewHasAnnotationBlockKey(root, activeAnnotationBlockKey)) {
|
||||
return { action: 'consume' }
|
||||
}
|
||||
// Fall through after clearing so a held/stale key does not permanently
|
||||
// suppress open. Repeat still must not open (below).
|
||||
if (event.repeat) {
|
||||
return { action: 'clear-stale-and-ignore' }
|
||||
}
|
||||
const blockKey = getMarkdownAnnotationBlockKeyForSelection(root, selection)
|
||||
if (blockKey) {
|
||||
return { action: 'open', blockKey }
|
||||
}
|
||||
return { action: 'clear-stale-and-ignore' }
|
||||
}
|
||||
|
||||
// Why: ignore OS key-repeat so a held chord cannot thrash open without a draft.
|
||||
if (event.repeat) {
|
||||
return { action: 'ignore' }
|
||||
}
|
||||
|
||||
const blockKey = getMarkdownAnnotationBlockKeyForSelection(root, selection)
|
||||
if (blockKey) {
|
||||
return { action: 'open', blockKey }
|
||||
}
|
||||
return { action: 'ignore' }
|
||||
}
|
||||
|
|
|
|||
|
|
@ -11,8 +11,17 @@ export function handleRichMarkdownAddReviewNoteShortcut(
|
|||
if (!editorShortcutMatches('editor.addReviewNote', event)) {
|
||||
return false
|
||||
}
|
||||
// Why: ignore OS key-repeat so a held chord cannot thrash open/remount.
|
||||
// Open drafts are consumed by installOpenDraftAddReviewNoteGuard on the
|
||||
// mounted composer (product B), including when focus is in the textarea.
|
||||
if (event.repeat) {
|
||||
return false
|
||||
}
|
||||
// Why: require the live selection so a collapsed selection cannot reopen a
|
||||
// stale target; consume the chord only when a composer actually opens.
|
||||
// stale target; consume only when a composer opens or an open draft is kept
|
||||
// (openAnnotationPopover returns true for both). openAnnotationPopover flushes
|
||||
// the pending ProseMirror selection before reading the target, so an immediate
|
||||
// chord after a mouse-drag sees the live selection rather than stale state.
|
||||
if (!ctx.openAnnotationPopoverRef.current(true)) {
|
||||
return false
|
||||
}
|
||||
|
|
|
|||
|
|
@ -143,6 +143,43 @@ describe('rich markdown key handler', () => {
|
|||
}
|
||||
})
|
||||
|
||||
it('ignores OS key-repeat for the add-review-note shortcut', () => {
|
||||
const editor = createEditor(emptyTopLevelOrderedList())
|
||||
|
||||
try {
|
||||
const ctx = createContext(editor, false)
|
||||
const event = keyEvent('a', { metaKey: true, shiftKey: true, code: 'KeyA', repeat: true })
|
||||
|
||||
// Why: leave the repeat unconsumed here; open drafts are consumed by the
|
||||
// mounted composer guard (product B) instead of this editor key path.
|
||||
expect(createRichMarkdownKeyHandler(ctx)(null, event)).toBe(false)
|
||||
expect(event.preventDefault).not.toHaveBeenCalled()
|
||||
expect(ctx.openAnnotationPopoverRef.current).not.toHaveBeenCalled()
|
||||
} finally {
|
||||
editor.destroy()
|
||||
}
|
||||
})
|
||||
|
||||
it('delegates a fresh add-review-note chord to openAnnotationPopover and consumes it', () => {
|
||||
const editor = createEditor(emptyTopLevelOrderedList())
|
||||
|
||||
try {
|
||||
const ctx = createContext(editor, false)
|
||||
// Why: the ProseMirror-selection flush moved into openAnnotationPopover
|
||||
// (which reads the selection), so the handler now only delegates the open
|
||||
// with requireLiveSelection and consumes the chord when it succeeds.
|
||||
ctx.openAnnotationPopoverRef.current = vi.fn(() => true)
|
||||
const event = keyEvent('a', { metaKey: true, shiftKey: true, code: 'KeyA' })
|
||||
|
||||
expect(createRichMarkdownKeyHandler(ctx)(null, event)).toBe(true)
|
||||
expect(event.preventDefault).toHaveBeenCalled()
|
||||
expect(ctx.openAnnotationPopoverRef.current).toHaveBeenCalledTimes(1)
|
||||
expect(ctx.openAnnotationPopoverRef.current).toHaveBeenCalledWith(true)
|
||||
} finally {
|
||||
editor.destroy()
|
||||
}
|
||||
})
|
||||
|
||||
it('preserves a typed empty ordered-list shortcut on Enter', () => {
|
||||
const editor = createEditor(emptyTopLevelOrderedList())
|
||||
|
||||
|
|
|
|||
|
|
@ -30,8 +30,8 @@ export function flushPendingProseMirrorSelection(editor: Editor): void {
|
|||
return
|
||||
}
|
||||
|
||||
// Why: immediate Tab after a mouse click can run before ProseMirror has
|
||||
// copied the native selection into editor state, so list commands hit stale item state.
|
||||
// Why: immediate Tab or add-review-note after a mouse click/drag can run
|
||||
// before ProseMirror has copied the native selection into editor state.
|
||||
observer.currentSelection?.set?.({
|
||||
anchorNode: null,
|
||||
anchorOffset: 0,
|
||||
|
|
|
|||
|
|
@ -0,0 +1,138 @@
|
|||
// @vitest-environment happy-dom
|
||||
import { act, renderHook } from '@testing-library/react'
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import type { Editor } from '@tiptap/react'
|
||||
import { useRichMarkdownReviewController } from './useRichMarkdownReviewController'
|
||||
import type { RichMarkdownAnnotationTarget } from './rich-markdown-review-annotations'
|
||||
|
||||
vi.mock('./useRichMarkdownReviewData', () => ({
|
||||
useRichMarkdownReviewData: () => ({
|
||||
canAnnotateRichMarkdown: true,
|
||||
markdownComments: [],
|
||||
markdownReviewNotes: [],
|
||||
sourceRelativePath: 'notes.md',
|
||||
unsentMarkdownReviewScope: null
|
||||
})
|
||||
}))
|
||||
|
||||
vi.mock('./useRichMarkdownReviewCopyFeedback', () => ({
|
||||
useRichMarkdownReviewCopyFeedback: () => ({
|
||||
clearReviewCopyTimers: vi.fn(),
|
||||
reviewCopyFeedback: null
|
||||
})
|
||||
}))
|
||||
|
||||
vi.mock('./useRichMarkdownReviewRailController', () => ({
|
||||
useRichMarkdownReviewRailController: () => ({
|
||||
cancelNotePositionFrame: vi.fn(),
|
||||
clearAttentionTimers: vi.fn(),
|
||||
setReviewRailOpen: vi.fn(),
|
||||
reviewRailOpen: false
|
||||
})
|
||||
}))
|
||||
|
||||
const sampleTarget: RichMarkdownAnnotationTarget = {
|
||||
from: 1,
|
||||
to: 4,
|
||||
lineNumber: 1,
|
||||
startLine: 1,
|
||||
selectedText: 'abc',
|
||||
top: 12,
|
||||
buttonTop: 12,
|
||||
buttonLeft: 8
|
||||
}
|
||||
|
||||
describe('useRichMarkdownReviewController openAnnotationPopover draft guard', () => {
|
||||
it('returns true without replacing an open draft (product B)', () => {
|
||||
const dispatch = vi.fn()
|
||||
const editorRef = {
|
||||
current: {
|
||||
view: { dispatch },
|
||||
state: {
|
||||
tr: {
|
||||
setMeta: vi.fn(function setMeta(this: unknown) {
|
||||
return this
|
||||
})
|
||||
}
|
||||
}
|
||||
} as unknown as Editor
|
||||
}
|
||||
const rootRef = { current: document.createElement('div') }
|
||||
const scrollContainerRef = { current: document.createElement('div') }
|
||||
|
||||
const { result } = renderHook(() =>
|
||||
useRichMarkdownReviewController({
|
||||
addDiffComment: vi.fn(),
|
||||
allDiffComments: [],
|
||||
content: 'abc',
|
||||
editorRef,
|
||||
filePath: '/repo/notes.md',
|
||||
markdownAnnotationsEnabled: true,
|
||||
markdownReviewContent: 'abc',
|
||||
markdownSourceLineOffset: 0,
|
||||
rootRef,
|
||||
scrollContainerRef,
|
||||
worktreeId: 'wt-1',
|
||||
worktreeRoot: '/repo'
|
||||
})
|
||||
)
|
||||
|
||||
act(() => {
|
||||
result.current.setAnnotationPopover(sampleTarget)
|
||||
})
|
||||
expect(result.current.annotationPopover).toEqual(sampleTarget)
|
||||
|
||||
let kept = false
|
||||
act(() => {
|
||||
kept = result.current.openAnnotationPopover(true)
|
||||
})
|
||||
|
||||
expect(kept).toBe(true)
|
||||
expect(result.current.annotationPopover).toEqual(sampleTarget)
|
||||
expect(dispatch).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('flushes the pending ProseMirror selection before reading the target', () => {
|
||||
const flush = vi.fn()
|
||||
const setSelection = vi.fn()
|
||||
// Why: an empty selection makes getRichMarkdownAnnotationTarget return null,
|
||||
// so the open no-ops; we only assert the flush ran first (drag-race fix).
|
||||
const editorRef = {
|
||||
current: {
|
||||
view: {
|
||||
dispatch: vi.fn(),
|
||||
domObserver: { flush, currentSelection: { set: setSelection } }
|
||||
},
|
||||
state: { selection: { empty: true } }
|
||||
} as unknown as Editor
|
||||
}
|
||||
const rootRef = { current: document.createElement('div') }
|
||||
const scrollContainerRef = { current: document.createElement('div') }
|
||||
|
||||
const { result } = renderHook(() =>
|
||||
useRichMarkdownReviewController({
|
||||
addDiffComment: vi.fn(),
|
||||
allDiffComments: [],
|
||||
content: 'abc',
|
||||
editorRef,
|
||||
filePath: '/repo/notes.md',
|
||||
markdownAnnotationsEnabled: true,
|
||||
markdownReviewContent: 'abc',
|
||||
markdownSourceLineOffset: 0,
|
||||
rootRef,
|
||||
scrollContainerRef,
|
||||
worktreeId: 'wt-1',
|
||||
worktreeRoot: '/repo'
|
||||
})
|
||||
)
|
||||
|
||||
let opened = true
|
||||
act(() => {
|
||||
opened = result.current.openAnnotationPopover(true)
|
||||
})
|
||||
|
||||
expect(flush).toHaveBeenCalledTimes(1)
|
||||
expect(opened).toBe(false)
|
||||
expect(result.current.annotationPopover).toBeNull()
|
||||
})
|
||||
})
|
||||
|
|
@ -11,6 +11,7 @@ import {
|
|||
type RichMarkdownAnnotationTarget
|
||||
} from './rich-markdown-review-annotations'
|
||||
import { shouldExpandRichMarkdownReviewRail } from './rich-markdown-review-note-layout'
|
||||
import { flushPendingProseMirrorSelection } from './rich-markdown-selection-flush'
|
||||
import { useRichMarkdownReviewData } from './useRichMarkdownReviewData'
|
||||
import { useRichMarkdownReviewCopyFeedback } from './useRichMarkdownReviewCopyFeedback'
|
||||
import { useRichMarkdownReviewRailController } from './useRichMarkdownReviewRailController'
|
||||
|
|
@ -201,15 +202,26 @@ export function useRichMarkdownReviewController({
|
|||
]
|
||||
)
|
||||
|
||||
// Why: reports whether a composer actually opened so keyboard callers can
|
||||
// leave the chord unconsumed when this no-ops (mouse callers ignore it).
|
||||
// Why: reports whether a composer actually opened (or an open draft was kept)
|
||||
// so keyboard callers can leave the chord unconsumed only on true no-ops.
|
||||
const openAnnotationPopover = useCallback(
|
||||
(requireLiveSelection = false): boolean => {
|
||||
if (!canAnnotateRichMarkdown) {
|
||||
return false
|
||||
}
|
||||
// Why: product B — second chord while drafting must consume the key without
|
||||
// remounting the composer and silently discarding the in-progress note.
|
||||
if (annotationPopoverRef.current) {
|
||||
return true
|
||||
}
|
||||
const editor = editorRef.current
|
||||
const root = rootRef.current
|
||||
// Why: an immediate chord after a mouse-drag can run before ProseMirror
|
||||
// copies the native selection into editor.state; flush it before reading
|
||||
// the target so the composer opens on the live selection, not stale state.
|
||||
if (editor) {
|
||||
flushPendingProseMirrorSelection(editor)
|
||||
}
|
||||
// Why: keyboard callers require the live selection to avoid stale-target
|
||||
// races; the mouse button may use the target from the render that exposed it.
|
||||
const liveTarget = editor && root ? getRichMarkdownAnnotationTarget(editor, root) : null
|
||||
|
|
@ -230,6 +242,9 @@ export function useRichMarkdownReviewController({
|
|||
activeRange: { from: target.from, to: target.to }
|
||||
})
|
||||
)
|
||||
// Why: claim the draft ref before setState so a same-tick second chord
|
||||
// (or OS key-repeat racing the open) cannot remount and discard text.
|
||||
annotationPopoverRef.current = target
|
||||
// Why: opening a draft should reserve the notes rail immediately; saved notes stay visible.
|
||||
setReviewRailOpen(true)
|
||||
setAnnotationPopover(target)
|
||||
|
|
|
|||
Loading…
Reference in New Issue