mirror of
https://github.com/stablyai/orca.git
synced 2026-10-07 00:02:29 +00:00
fix(diff-comments): prevent draft loss and duplicate submission on swap
- Track submission state to prevent carrying in-flight text to new cards - Restore failed submissions for retry after model swap with unmount safety - Use effects for proper ref management per React patterns - Add isDraftOpen() guard to prevent re-opening the keyboard chord while composing - Separate concerns between user clicks and draft-open state in decorator
This commit is contained in:
@@ -44,13 +44,19 @@ export function DiffCommentDraftCard({
|
||||
}: DiffCommentDraftCardProps): React.JSX.Element {
|
||||
const [body, setBody] = useState(initialBody)
|
||||
const bodyRef = useRef(body)
|
||||
bodyRef.current = body
|
||||
const [submitting, setSubmitting] = useState(false)
|
||||
const mountedRef = useMountedRef()
|
||||
const textareaRef = useRef<HTMLTextAreaElement | null>(null)
|
||||
const cardRef = useRef<HTMLDivElement | null>(null)
|
||||
const onContentResizeRef = useRef(onContentResize)
|
||||
onContentResizeRef.current = onContentResize
|
||||
|
||||
useEffect(() => {
|
||||
bodyRef.current = body
|
||||
}, [body])
|
||||
|
||||
useEffect(() => {
|
||||
onContentResizeRef.current = onContentResize
|
||||
}, [onContentResize])
|
||||
|
||||
const labelId = useId()
|
||||
const headerLabel =
|
||||
|
||||
@@ -55,8 +55,9 @@ export function installDiffCommentAddNoteShortcut({
|
||||
onOpenComposer: (args: { lineNumber: number; startLine?: number; top: number }) => void
|
||||
}): () => void {
|
||||
return installEditorAddReviewNoteShortcut(editor.getContainerDomNode(), () => {
|
||||
// Why: an open draft consumes the chord itself (DiffCommentPopover's guard); claiming it here
|
||||
// too would remount the composer and drop what the user already typed.
|
||||
// Why: an open draft card owns the chord (isComposerOpen reports it, and the card's own guard
|
||||
// eats presses from its textarea); claiming it here too would re-open the card at the editor's
|
||||
// selection and move what the user already typed.
|
||||
if (isComposerOpen()) {
|
||||
return true
|
||||
}
|
||||
|
||||
@@ -0,0 +1,236 @@
|
||||
// @vitest-environment happy-dom
|
||||
import { act, fireEvent, renderHook, within } from '@testing-library/react'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import {
|
||||
useDiffCommentDraftZone,
|
||||
type UseDiffCommentDraftZoneArgs
|
||||
} from './diff-comment-draft-zone'
|
||||
import {
|
||||
createFakeDiffCommentEditor,
|
||||
type FakeDiffCommentEditor
|
||||
} from './diff-comment-editor-test-fixture'
|
||||
|
||||
const DRAFT_LINE = 5
|
||||
const OTHER_LINE = 9
|
||||
const BODY = 'Needs revision'
|
||||
|
||||
type CreateComment = NonNullable<UseDiffCommentDraftZoneArgs['onCreateComment']>
|
||||
|
||||
function renderDraftZone(fake: FakeDiffCommentEditor, onCreateComment: CreateComment) {
|
||||
return renderHook(
|
||||
({ monacoModelIdentity }: { monacoModelIdentity: string }) =>
|
||||
useDiffCommentDraftZone({ editor: fake.editor, monacoModelIdentity, onCreateComment }),
|
||||
{ initialProps: { monacoModelIdentity: 'model-v1' } }
|
||||
)
|
||||
}
|
||||
|
||||
function openDraftAt(hook: ReturnType<typeof renderDraftZone>, lineNumber: number): void {
|
||||
act(() => {
|
||||
hook.result.current.onAddCommentClickRef.current({ lineNumber, top: 0 })
|
||||
})
|
||||
}
|
||||
|
||||
// The single open draft card, attached to the editor node the way Monaco would mount its zone.
|
||||
function draftCard(fake: FakeDiffCommentEditor): {
|
||||
dom: HTMLElement
|
||||
textarea: HTMLTextAreaElement
|
||||
} {
|
||||
const zones = [...fake.zones.values()]
|
||||
expect(zones).toHaveLength(1)
|
||||
const dom = zones[0].domNode
|
||||
if (!dom.isConnected) {
|
||||
fake.domNode.appendChild(dom)
|
||||
}
|
||||
const textarea = dom.querySelector('textarea')
|
||||
if (!(textarea instanceof HTMLTextAreaElement)) {
|
||||
throw new Error('draft card did not render a textarea')
|
||||
}
|
||||
return { dom, textarea }
|
||||
}
|
||||
|
||||
function typeDraft(fake: FakeDiffCommentEditor, body: string): void {
|
||||
fireEvent.change(draftCard(fake).textarea, { target: { value: body } })
|
||||
}
|
||||
|
||||
function submitDraft(fake: FakeDiffCommentEditor, body: string): void {
|
||||
typeDraft(fake, body)
|
||||
fireEvent.click(within(draftCard(fake).dom).getByRole('button', { name: 'Add note' }))
|
||||
}
|
||||
|
||||
function deferredCreateComment(): {
|
||||
onCreateComment: CreateComment & ReturnType<typeof vi.fn>
|
||||
settle: (result: boolean) => Promise<void>
|
||||
} {
|
||||
let resolve: (result: boolean) => void = () => {}
|
||||
const onCreateComment = vi.fn(
|
||||
() =>
|
||||
new Promise<boolean>((r) => {
|
||||
resolve = r
|
||||
})
|
||||
)
|
||||
return {
|
||||
onCreateComment,
|
||||
settle: (result) =>
|
||||
act(async () => {
|
||||
resolve(result)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
const frames = new Map<number, FrameRequestCallback>()
|
||||
let nextFrameId = 0
|
||||
|
||||
function pumpFrames(): void {
|
||||
const pending = [...frames.values()]
|
||||
frames.clear()
|
||||
act(() => {
|
||||
for (const callback of pending) {
|
||||
callback(16)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
frames.clear()
|
||||
vi.stubGlobal('requestAnimationFrame', (callback: FrameRequestCallback): number => {
|
||||
nextFrameId += 1
|
||||
frames.set(nextFrameId, callback)
|
||||
return nextFrameId
|
||||
})
|
||||
vi.stubGlobal('cancelAnimationFrame', (id: number) => {
|
||||
frames.delete(id)
|
||||
})
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.unstubAllGlobals()
|
||||
document.body.replaceChildren()
|
||||
vi.clearAllMocks()
|
||||
})
|
||||
|
||||
describe('useDiffCommentDraftZone re-anchoring', () => {
|
||||
it('keeps the carried body when a second model swap lands before the re-anchor frame', () => {
|
||||
const fake = createFakeDiffCommentEditor()
|
||||
const hook = renderDraftZone(fake, vi.fn().mockResolvedValue(true))
|
||||
openDraftAt(hook, DRAFT_LINE)
|
||||
typeDraft(fake, BODY)
|
||||
|
||||
hook.rerender({ monacoModelIdentity: 'model-v2' })
|
||||
hook.rerender({ monacoModelIdentity: 'model-v3' })
|
||||
expect(fake.zones.size).toBe(0)
|
||||
pumpFrames()
|
||||
|
||||
const { textarea } = draftCard(fake)
|
||||
expect(textarea.value).toBe(BODY)
|
||||
expect([...fake.zones.values()][0].afterLineNumber).toBe(DRAFT_LINE)
|
||||
expect(hook.result.current.isDraftOpen()).toBe(true)
|
||||
})
|
||||
|
||||
it('lets a click on another line win over a scheduled re-anchor', () => {
|
||||
const fake = createFakeDiffCommentEditor()
|
||||
const hook = renderDraftZone(fake, vi.fn().mockResolvedValue(true))
|
||||
openDraftAt(hook, DRAFT_LINE)
|
||||
typeDraft(fake, BODY)
|
||||
hook.rerender({ monacoModelIdentity: 'model-v2' })
|
||||
|
||||
openDraftAt(hook, OTHER_LINE)
|
||||
expect(draftCard(fake).textarea.value).toBe(BODY)
|
||||
pumpFrames()
|
||||
|
||||
const zones = [...fake.zones.values()]
|
||||
expect(zones).toHaveLength(1)
|
||||
expect(zones[0].afterLineNumber).toBe(OTHER_LINE)
|
||||
expect(draftCard(fake).textarea.value).toBe(BODY)
|
||||
})
|
||||
})
|
||||
|
||||
describe('useDiffCommentDraftZone in-flight submit', () => {
|
||||
it('does not carry a submitting draft across a model swap and clears it once the save lands', async () => {
|
||||
const fake = createFakeDiffCommentEditor()
|
||||
const { onCreateComment, settle } = deferredCreateComment()
|
||||
const hook = renderDraftZone(fake, onCreateComment)
|
||||
openDraftAt(hook, DRAFT_LINE)
|
||||
submitDraft(fake, BODY)
|
||||
expect(onCreateComment).toHaveBeenCalledTimes(1)
|
||||
|
||||
hook.rerender({ monacoModelIdentity: 'model-v2' })
|
||||
pumpFrames()
|
||||
// The in-flight save owns the text: no replacement card that could submit it a second time.
|
||||
expect(fake.zones.size).toBe(0)
|
||||
|
||||
await settle(true)
|
||||
pumpFrames()
|
||||
expect(fake.zones.size).toBe(0)
|
||||
expect(hook.result.current.isDraftOpen()).toBe(false)
|
||||
expect(onCreateComment).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('brings the draft back for retry when its save fails after a model swap', async () => {
|
||||
const fake = createFakeDiffCommentEditor()
|
||||
const { onCreateComment, settle } = deferredCreateComment()
|
||||
const hook = renderDraftZone(fake, onCreateComment)
|
||||
openDraftAt(hook, DRAFT_LINE)
|
||||
submitDraft(fake, BODY)
|
||||
hook.rerender({ monacoModelIdentity: 'model-v2' })
|
||||
expect(fake.zones.size).toBe(0)
|
||||
|
||||
await settle(false)
|
||||
|
||||
const { textarea } = draftCard(fake)
|
||||
expect(textarea.value).toBe(BODY)
|
||||
expect([...fake.zones.values()][0].afterLineNumber).toBe(DRAFT_LINE)
|
||||
expect(hook.result.current.isDraftOpen()).toBe(true)
|
||||
})
|
||||
|
||||
it('does not carry a submitting body into a draft opened on another line', async () => {
|
||||
const fake = createFakeDiffCommentEditor()
|
||||
const { onCreateComment, settle } = deferredCreateComment()
|
||||
const hook = renderDraftZone(fake, onCreateComment)
|
||||
openDraftAt(hook, DRAFT_LINE)
|
||||
submitDraft(fake, BODY)
|
||||
|
||||
openDraftAt(hook, OTHER_LINE)
|
||||
expect(draftCard(fake).textarea.value).toBe('')
|
||||
|
||||
await settle(true)
|
||||
const zones = [...fake.zones.values()]
|
||||
expect(zones).toHaveLength(1)
|
||||
expect(zones[0].afterLineNumber).toBe(OTHER_LINE)
|
||||
expect(onCreateComment).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
})
|
||||
|
||||
describe('useDiffCommentDraftZone open state', () => {
|
||||
it('reports the draft closed again after cancel so the chord is not deadened', () => {
|
||||
const fake = createFakeDiffCommentEditor()
|
||||
const hook = renderDraftZone(fake, vi.fn().mockResolvedValue(true))
|
||||
openDraftAt(hook, DRAFT_LINE)
|
||||
expect(hook.result.current.isDraftOpen()).toBe(true)
|
||||
|
||||
fireEvent.click(within(draftCard(fake).dom).getByRole('button', { name: 'Cancel' }))
|
||||
|
||||
expect(hook.result.current.isDraftOpen()).toBe(false)
|
||||
expect(fake.zones.size).toBe(0)
|
||||
|
||||
openDraftAt(hook, OTHER_LINE)
|
||||
expect(hook.result.current.isDraftOpen()).toBe(true)
|
||||
expect([...fake.zones.values()][0].afterLineNumber).toBe(OTHER_LINE)
|
||||
})
|
||||
})
|
||||
|
||||
describe('useDiffCommentDraftZone teardown', () => {
|
||||
it('does not resurrect a card when a failed save settles after unmount', async () => {
|
||||
const fake = createFakeDiffCommentEditor()
|
||||
const { onCreateComment, settle } = deferredCreateComment()
|
||||
const hook = renderDraftZone(fake, onCreateComment)
|
||||
openDraftAt(hook, DRAFT_LINE)
|
||||
submitDraft(fake, BODY)
|
||||
|
||||
hook.unmount()
|
||||
expect(fake.zones.size).toBe(0)
|
||||
|
||||
await settle(false)
|
||||
pumpFrames()
|
||||
expect(fake.zones.size).toBe(0)
|
||||
})
|
||||
})
|
||||
@@ -14,6 +14,8 @@ export type DiffCommentDraft = {
|
||||
startLine?: number
|
||||
}
|
||||
|
||||
type CarriedDraft = { draft: DiffCommentDraft; body: string }
|
||||
|
||||
export type UseDiffCommentDraftZoneArgs = {
|
||||
editor: monacoEditor.ICodeEditor | null
|
||||
monacoModelIdentity?: string
|
||||
@@ -30,6 +32,7 @@ export type UseDiffCommentDraftZoneArgs = {
|
||||
|
||||
export type DiffCommentDraftZoneHandle = {
|
||||
disposeDraftZone: (focusEditor?: boolean) => void
|
||||
isDraftOpen: () => boolean
|
||||
onAddCommentClickRef: React.RefObject<
|
||||
(args: { lineNumber: number; startLine?: number; top: number }) => void
|
||||
>
|
||||
@@ -45,17 +48,40 @@ export function useDiffCommentDraftZone({
|
||||
onAddCommentClick
|
||||
}: UseDiffCommentDraftZoneArgs): DiffCommentDraftZoneHandle {
|
||||
const draftZoneRef = useRef<DraftZoneEntry | null>(null)
|
||||
const pendingDraftRef = useRef<{ draft: DiffCommentDraft; body: string } | null>(null)
|
||||
// The only copy of a draft between its card being torn down and the replacement mounting.
|
||||
const pendingDraftRef = useRef<CarriedDraft | null>(null)
|
||||
const previousModelIdentityRef = useRef(monacoModelIdentity)
|
||||
const reanchorFrameRef = useRef<number | null>(null)
|
||||
const onAddCommentClickRef = useRef<
|
||||
(args: { lineNumber: number; startLine?: number; top: number }) => void
|
||||
>(() => {})
|
||||
|
||||
// A save can settle after unmount; without this its retry would mount a card nobody disposes.
|
||||
const unmountedRef = useRef(false)
|
||||
const onCreateCommentRef = useRef(onCreateComment)
|
||||
const onLegacyAddCommentClickRef = useRef(onAddCommentClick)
|
||||
onCreateCommentRef.current = onCreateComment
|
||||
onLegacyAddCommentClickRef.current = onAddCommentClick
|
||||
|
||||
useEffect(() => {
|
||||
unmountedRef.current = false
|
||||
return () => {
|
||||
unmountedRef.current = true
|
||||
}
|
||||
}, [])
|
||||
|
||||
useEffect(() => {
|
||||
onCreateCommentRef.current = onCreateComment
|
||||
onLegacyAddCommentClickRef.current = onAddCommentClick
|
||||
}, [onAddCommentClick, onCreateComment])
|
||||
|
||||
const isDraftOpen = useCallback((): boolean => draftZoneRef.current !== null, [])
|
||||
|
||||
const cancelReanchorFrame = useCallback((): void => {
|
||||
if (reanchorFrameRef.current === null) {
|
||||
return
|
||||
}
|
||||
cancelAnimationFrame(reanchorFrameRef.current)
|
||||
reanchorFrameRef.current = null
|
||||
}, [])
|
||||
|
||||
const disposeDraftZone = useCallback((focusEditor = false): void => {
|
||||
const entry = draftZoneRef.current
|
||||
@@ -73,10 +99,24 @@ export function useDiffCommentDraftZone({
|
||||
}
|
||||
}, [])
|
||||
|
||||
// Latest openDraft for callers that outlive a render: the re-anchor frame and a settling submit.
|
||||
const openDraftRef = useRef<(draft: DiffCommentDraft, initialBody?: string) => boolean>(
|
||||
() => false
|
||||
)
|
||||
|
||||
// A save that failed after its card was torn down (model swap, editor refresh) brings the text
|
||||
// back for a retry; an open card means the user already moved on, and the failure toast covers it.
|
||||
const restoreFailedSubmit = useCallback((draft: DiffCommentDraft, body: string): void => {
|
||||
if (unmountedRef.current || draftZoneRef.current || openDraftRef.current(draft, body)) {
|
||||
return
|
||||
}
|
||||
pendingDraftRef.current = { draft, body }
|
||||
}, [])
|
||||
|
||||
const openDraft = useCallback(
|
||||
(draft: DiffCommentDraft, initialBody = ''): void => {
|
||||
(draft: DiffCommentDraft, initialBody = ''): boolean => {
|
||||
if (!editor || !editor.getModel() || !onCreateCommentRef.current || !canOpenDraft) {
|
||||
return
|
||||
return false
|
||||
}
|
||||
|
||||
disposeDraftZone()
|
||||
@@ -117,6 +157,7 @@ export function useDiffCommentDraftZone({
|
||||
root,
|
||||
draft,
|
||||
body: initialBody,
|
||||
submitting: false,
|
||||
disposeMouseDownStopper: () => {
|
||||
disposeDomMouseDownStopper()
|
||||
disposeMarginMouseDownStopper()
|
||||
@@ -144,63 +185,108 @@ export function useDiffCommentDraftZone({
|
||||
if (!createComment) {
|
||||
return false
|
||||
}
|
||||
const result = await createComment({
|
||||
lineNumber: draft.lineNumber,
|
||||
startLine: draft.startLine,
|
||||
body
|
||||
})
|
||||
if (result !== false && draftZoneRef.current === entry) {
|
||||
disposeDraftZone()
|
||||
entry.submitting = true
|
||||
let succeeded = false
|
||||
try {
|
||||
const result = await createComment({
|
||||
lineNumber: draft.lineNumber,
|
||||
startLine: draft.startLine,
|
||||
body
|
||||
})
|
||||
succeeded = result !== false
|
||||
return result
|
||||
} finally {
|
||||
entry.submitting = false
|
||||
if (succeeded) {
|
||||
if (draftZoneRef.current === entry) {
|
||||
disposeDraftZone()
|
||||
}
|
||||
} else if (draftZoneRef.current !== entry) {
|
||||
restoreFailedSubmit(draft, body)
|
||||
}
|
||||
}
|
||||
return result
|
||||
}
|
||||
})
|
||||
})
|
||||
// Report what actually mounted: the re-anchor frame drops its only copy of the draft on true.
|
||||
return draftZoneRef.current !== null
|
||||
},
|
||||
[canOpenDraft, disposeDraftZone, draftPlaceholder, draftSubmitLabel, editor]
|
||||
[
|
||||
canOpenDraft,
|
||||
disposeDraftZone,
|
||||
draftPlaceholder,
|
||||
draftSubmitLabel,
|
||||
editor,
|
||||
restoreFailedSubmit
|
||||
]
|
||||
)
|
||||
onAddCommentClickRef.current = (args) => {
|
||||
if (!canOpenDraft) {
|
||||
|
||||
useEffect(() => {
|
||||
openDraftRef.current = openDraft
|
||||
}, [openDraft])
|
||||
|
||||
// The stash is only cleared once a card actually mounts: the editor may still be mid-refresh
|
||||
// when the frame fires, and a later editor/identity change re-schedules from the same stash.
|
||||
const scheduleReanchor = useCallback((): void => {
|
||||
if (reanchorFrameRef.current !== null) {
|
||||
return
|
||||
}
|
||||
const current = draftZoneRef.current
|
||||
const pending = pendingDraftRef.current
|
||||
const carriedBody = current?.body || pending?.body || ''
|
||||
if (onCreateCommentRef.current) {
|
||||
openDraft({ lineNumber: args.lineNumber, startLine: args.startLine }, carriedBody)
|
||||
return
|
||||
}
|
||||
onLegacyAddCommentClickRef.current?.(args)
|
||||
}
|
||||
reanchorFrameRef.current = requestAnimationFrame(() => {
|
||||
reanchorFrameRef.current = null
|
||||
const pending = pendingDraftRef.current
|
||||
if (pending && openDraftRef.current(pending.draft, pending.body)) {
|
||||
pendingDraftRef.current = null
|
||||
}
|
||||
})
|
||||
}, [])
|
||||
|
||||
const openDraftFromArgs = useCallback(
|
||||
(args: { lineNumber: number; startLine?: number; top: number }): void => {
|
||||
if (!canOpenDraft) {
|
||||
return
|
||||
}
|
||||
if (!onCreateCommentRef.current) {
|
||||
onLegacyAddCommentClickRef.current?.(args)
|
||||
return
|
||||
}
|
||||
const current = draftZoneRef.current
|
||||
const pending = pendingDraftRef.current
|
||||
// A submitting card's text is already on its way to the store; carrying it would post it twice.
|
||||
const carriedBody =
|
||||
(current && !current.submitting ? current.body : '') || pending?.body || ''
|
||||
// The user picked a new anchor, so a scheduled re-anchor must not move the card back afterwards.
|
||||
cancelReanchorFrame()
|
||||
pendingDraftRef.current = null
|
||||
if (!openDraft({ lineNumber: args.lineNumber, startLine: args.startLine }, carriedBody)) {
|
||||
pendingDraftRef.current = pending
|
||||
}
|
||||
},
|
||||
[canOpenDraft, cancelReanchorFrame, openDraft]
|
||||
)
|
||||
|
||||
useEffect(() => {
|
||||
onAddCommentClickRef.current = openDraftFromArgs
|
||||
}, [openDraftFromArgs])
|
||||
|
||||
useEffect(() => {
|
||||
if (previousModelIdentityRef.current === monacoModelIdentity) {
|
||||
return
|
||||
}
|
||||
previousModelIdentityRef.current = monacoModelIdentity
|
||||
if (reanchorFrameRef.current !== null) {
|
||||
cancelAnimationFrame(reanchorFrameRef.current)
|
||||
reanchorFrameRef.current = null
|
||||
pendingDraftRef.current = null
|
||||
}
|
||||
cancelReanchorFrame()
|
||||
const current = draftZoneRef.current
|
||||
if (!current) {
|
||||
return
|
||||
}
|
||||
pendingDraftRef.current = { draft: current.draft, body: current.body }
|
||||
disposeDraftZone()
|
||||
if (!editor || !editor.getModel() || !onCreateCommentRef.current || !canOpenDraft) {
|
||||
return
|
||||
}
|
||||
reanchorFrameRef.current = requestAnimationFrame(() => {
|
||||
reanchorFrameRef.current = null
|
||||
const pending = pendingDraftRef.current
|
||||
pendingDraftRef.current = null
|
||||
if (pending) {
|
||||
openDraft(pending.draft, pending.body)
|
||||
if (current) {
|
||||
// A save in flight owns its text: the settling submit disposes on success and re-opens on
|
||||
// failure, so carrying it here would mount a second card that could submit the same note.
|
||||
if (!current.submitting) {
|
||||
pendingDraftRef.current = { draft: current.draft, body: current.body }
|
||||
}
|
||||
})
|
||||
}, [canOpenDraft, disposeDraftZone, editor, monacoModelIdentity, openDraft])
|
||||
disposeDraftZone()
|
||||
}
|
||||
if (pendingDraftRef.current) {
|
||||
scheduleReanchor()
|
||||
}
|
||||
}, [cancelReanchorFrame, disposeDraftZone, monacoModelIdentity, scheduleReanchor])
|
||||
|
||||
// A combined-diff model refresh can briefly clear the editor ref before the replacement mounts.
|
||||
// Re-anchor any carried draft when that replacement becomes available.
|
||||
@@ -208,38 +294,23 @@ export function useDiffCommentDraftZone({
|
||||
if (!editor || !canOpenDraft || !pendingDraftRef.current) {
|
||||
return
|
||||
}
|
||||
const pending = pendingDraftRef.current
|
||||
pendingDraftRef.current = null
|
||||
reanchorFrameRef.current = requestAnimationFrame(() => {
|
||||
reanchorFrameRef.current = null
|
||||
openDraft(pending.draft, pending.body)
|
||||
})
|
||||
return () => {
|
||||
if (reanchorFrameRef.current !== null) {
|
||||
cancelAnimationFrame(reanchorFrameRef.current)
|
||||
reanchorFrameRef.current = null
|
||||
}
|
||||
if (!draftZoneRef.current) {
|
||||
pendingDraftRef.current = pending
|
||||
}
|
||||
}
|
||||
}, [canOpenDraft, editor, openDraft])
|
||||
scheduleReanchor()
|
||||
return cancelReanchorFrame
|
||||
}, [cancelReanchorFrame, canOpenDraft, editor, scheduleReanchor])
|
||||
|
||||
useEffect(() => {
|
||||
if (!editor) {
|
||||
return
|
||||
}
|
||||
return () => {
|
||||
if (reanchorFrameRef.current !== null) {
|
||||
cancelAnimationFrame(reanchorFrameRef.current)
|
||||
reanchorFrameRef.current = null
|
||||
}
|
||||
cancelReanchorFrame()
|
||||
disposeDraftZone(false)
|
||||
}
|
||||
}, [disposeDraftZone, editor])
|
||||
}, [cancelReanchorFrame, disposeDraftZone, editor])
|
||||
|
||||
return {
|
||||
disposeDraftZone,
|
||||
isDraftOpen,
|
||||
onAddCommentClickRef
|
||||
}
|
||||
}
|
||||
|
||||
@@ -22,6 +22,8 @@ export type DraftZoneEntry = {
|
||||
root: Root
|
||||
draft: { lineNumber: number; startLine?: number }
|
||||
body: string
|
||||
// A save is in flight: the body must not be carried to another card, or the note could post twice.
|
||||
submitting: boolean
|
||||
disposeMouseDownStopper: () => void
|
||||
}
|
||||
|
||||
|
||||
+45
-13
@@ -1,5 +1,5 @@
|
||||
// @vitest-environment happy-dom
|
||||
import { renderHook } from '@testing-library/react'
|
||||
import { act, renderHook } from '@testing-library/react'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type { Selection } from 'monaco-editor'
|
||||
import type * as DiffCommentZoneCardModule from './diff-comment-zone-card'
|
||||
@@ -37,6 +37,12 @@ type DecoratorProps = {
|
||||
commentableLineNumbers?: readonly number[]
|
||||
addNoteShortcutEnabled?: boolean
|
||||
onAddCommentClick?: (args: { lineNumber: number; startLine?: number; top: number }) => void
|
||||
// Present on the inline-draft surfaces; absent for the legacy popover callers.
|
||||
onCreateComment?: (args: {
|
||||
lineNumber: number
|
||||
startLine?: number
|
||||
body: string
|
||||
}) => Promise<boolean>
|
||||
}
|
||||
|
||||
function renderDecorator(fake: FakeDiffCommentEditor, initialProps: DecoratorProps = {}) {
|
||||
@@ -51,6 +57,7 @@ function renderDecorator(fake: FakeDiffCommentEditor, initialProps: DecoratorPro
|
||||
pendingCommentTarget: props.pendingCommentTarget ?? null,
|
||||
addNoteShortcutEnabled: props.addNoteShortcutEnabled ?? false,
|
||||
onAddCommentClick: props.onAddCommentClick ?? vi.fn(),
|
||||
onCreateComment: props.onCreateComment,
|
||||
onDeleteComment: vi.fn()
|
||||
}),
|
||||
{ initialProps }
|
||||
@@ -102,19 +109,19 @@ function firePointerEvent(
|
||||
}
|
||||
|
||||
// Mod+Shift+A on the platform this test's user agent reports.
|
||||
function pressAddReviewNoteChord(node: HTMLElement): void {
|
||||
function pressAddReviewNoteChord(node: HTMLElement): KeyboardEvent {
|
||||
const isMac = navigator.userAgent.includes('Mac')
|
||||
node.dispatchEvent(
|
||||
new KeyboardEvent('keydown', {
|
||||
key: 'A',
|
||||
code: 'KeyA',
|
||||
shiftKey: true,
|
||||
metaKey: isMac,
|
||||
ctrlKey: !isMac,
|
||||
bubbles: true,
|
||||
cancelable: true
|
||||
})
|
||||
)
|
||||
const event = new KeyboardEvent('keydown', {
|
||||
key: 'A',
|
||||
code: 'KeyA',
|
||||
shiftKey: true,
|
||||
metaKey: isMac,
|
||||
ctrlKey: !isMac,
|
||||
bubbles: true,
|
||||
cancelable: true
|
||||
})
|
||||
node.dispatchEvent(event)
|
||||
return event
|
||||
}
|
||||
|
||||
const frameCallbacks: FrameRequestCallback[] = []
|
||||
@@ -377,4 +384,29 @@ describe('useDiffCommentDecorator add-note chord', () => {
|
||||
|
||||
expect(onAddCommentClick).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('leaves the chord to an open inline draft card instead of re-anchoring it', () => {
|
||||
const fake = createFakeDiffCommentEditor()
|
||||
vi.spyOn(fake.editor, 'getSelection').mockReturnValue(selectionOf(9, 1, 14, 8))
|
||||
renderDecorator(fake, {
|
||||
addNoteShortcutEnabled: true,
|
||||
onCreateComment: vi.fn().mockResolvedValue(true)
|
||||
})
|
||||
|
||||
act(() => {
|
||||
pressAddReviewNoteChord(fake.domNode)
|
||||
})
|
||||
const [zone] = [...fake.zones.values()]
|
||||
expect(zone?.afterLineNumber).toBe(14)
|
||||
|
||||
// The selection moved on, but the open card must keep its anchor and its text.
|
||||
vi.spyOn(fake.editor, 'getSelection').mockReturnValue(selectionOf(30, 1, 32, 4))
|
||||
const secondChord: { event?: KeyboardEvent } = {}
|
||||
act(() => {
|
||||
secondChord.event = pressAddReviewNoteChord(fake.domNode)
|
||||
})
|
||||
|
||||
expect(secondChord.event?.defaultPrevented).toBe(true)
|
||||
expect([...fake.zones.values()]).toEqual([zone])
|
||||
})
|
||||
})
|
||||
|
||||
@@ -102,7 +102,7 @@ export function useDiffCommentDecorator({
|
||||
onUpdateCommentRef.current = onUpdateComment
|
||||
onPendingScrollConsumedRef.current = onPendingScrollConsumed
|
||||
|
||||
const { onAddCommentClickRef } = useDiffCommentDraftZone({
|
||||
const { onAddCommentClickRef, isDraftOpen } = useDiffCommentDraftZone({
|
||||
editor,
|
||||
monacoModelIdentity,
|
||||
onCreateComment,
|
||||
@@ -172,6 +172,7 @@ export function useDiffCommentDecorator({
|
||||
setPendingCommentRange(range)
|
||||
}, [pendingLineNumber, pendingStartLine, setPendingCommentRange])
|
||||
|
||||
const hasDraftComposer = onCreateComment !== undefined
|
||||
useEffect(() => {
|
||||
if (!editor || !addNoteShortcutEnabled) {
|
||||
return
|
||||
@@ -179,11 +180,24 @@ export function useDiffCommentDecorator({
|
||||
return installDiffCommentAddNoteShortcut({
|
||||
editor,
|
||||
commentableLineSet,
|
||||
// A live gutter drag owns the band; opening from the stale editor selection during a drag
|
||||
// would remount the composer before the gesture completes.
|
||||
// An open draft card owns the chord: claiming it here would re-open the card at the editor's
|
||||
// selection and move the user's text. The card mounts synchronously, so a second chord in the
|
||||
// same event turn already sees it. A live gutter drag owns the band the same way; opening
|
||||
// from the stale editor selection during a drag would remount the card mid-gesture.
|
||||
isComposerOpen: () =>
|
||||
pendingCommentRangeRef.current !== null || overlayRef.current?.isDragging() === true,
|
||||
isDraftOpen() ||
|
||||
pendingCommentRangeRef.current !== null ||
|
||||
overlayRef.current?.isDragging() === true,
|
||||
onOpenComposer: (args) => {
|
||||
// Legacy popover callers commit composer state through React, so claim now or a same-turn
|
||||
// repeat opens a second one; their pendingCommentTarget releases the claim on close. The
|
||||
// inline draft card never passes one, so a claim here would never be released.
|
||||
if (!hasDraftComposer) {
|
||||
setPendingCommentRange({
|
||||
startLine: args.startLine ?? args.lineNumber,
|
||||
endLine: args.lineNumber
|
||||
})
|
||||
}
|
||||
onAddCommentClickRef.current(args)
|
||||
}
|
||||
})
|
||||
@@ -191,7 +205,10 @@ export function useDiffCommentDecorator({
|
||||
addNoteShortcutEnabled,
|
||||
commentableLineSet,
|
||||
editor,
|
||||
hasDraftComposer,
|
||||
isDraftOpen,
|
||||
monacoModelIdentity,
|
||||
onAddCommentClickRef,
|
||||
setPendingCommentRange
|
||||
])
|
||||
|
||||
|
||||
@@ -8,6 +8,7 @@ import { useContextualCopySetup } from './useContextualCopySetup'
|
||||
import { selectWorktreeDiffComments } from '@/store/worktree-diff-comments-selector'
|
||||
import { useDiffCommentDecorator } from '../diff-comments/useDiffCommentDecorator'
|
||||
import { toast } from 'sonner'
|
||||
import { translate } from '@/i18n/i18n'
|
||||
import { applyDiffEditorLineNumberOptions } from './diff-editor-line-number-options'
|
||||
import type { DiffComment } from '../../../../shared/diff-comment-types'
|
||||
import { isDiffComment } from '@/lib/diff-comment-compat'
|
||||
@@ -115,7 +116,9 @@ export default function DiffViewer({
|
||||
side: 'modified'
|
||||
})
|
||||
if (!result) {
|
||||
toast.error('Failed to save comment')
|
||||
toast.error(
|
||||
translate('auto.components.editor.diffCommentSaveFailed', 'Failed to save comment')
|
||||
)
|
||||
}
|
||||
return Boolean(result)
|
||||
},
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import type { DiffSection } from './diff-section-types'
|
||||
import { toast } from 'sonner'
|
||||
import { translate } from '@/i18n/i18n'
|
||||
|
||||
type DiffSectionCommentTarget = {
|
||||
lineNumber: number
|
||||
@@ -60,7 +61,7 @@ export async function submitDiffSectionComment({
|
||||
side: 'modified'
|
||||
})
|
||||
if (!result) {
|
||||
toast.error('Failed to save comment')
|
||||
toast.error(translate('auto.components.editor.diffCommentSaveFailed', 'Failed to save comment'))
|
||||
}
|
||||
return Boolean(result)
|
||||
}
|
||||
|
||||
@@ -14845,6 +14845,7 @@
|
||||
"f1aa04b5cf": "This file",
|
||||
"8b87612461": "All unsent notes"
|
||||
},
|
||||
"diffCommentSaveFailed": "Failed to save comment",
|
||||
"DiffSectionBody": {
|
||||
"35d6afb5be": "Binary file changed",
|
||||
"cef4cf0ff5": "Retry",
|
||||
|
||||
Reference in New Issue
Block a user