From 2d9542cd301f0fdfef3684e00305d0da249f6a0c Mon Sep 17 00:00:00 2001 From: averydev Date: Sat, 12 Sep 2026 14:01:30 -0700 Subject: [PATCH] fix(editor): retry the whole-text patch and refuse escaped dollars as math closers Second-round review findings that survived the mark rewrite: - A source whose last line is whitespace only (`Last.\n \n`) has a trailing run of one newline, so the body strip ran, landed the end hunk after the spaces, failed the branch-6 proof, and canonicalized the whole file where the base branch had patched it correctly. The body strip is now the first attempt and the whole-text patch the second, so no file does worse than before; the extra round trip only runs when the first attempt fails. - `costs $5 to \$x here` parsed as inline math with latex `5 to \`, because the tokenizer accepted an escaped `\$` as the closing delimiter. Escaped characters are now consumed inside the formula and cannot close it. - The escaped-dollar test passed on the base branch for the wrong reason (the `\$` was deleted before the math tokenizer ran); it now also asserts the bytes round-trip, and a new test pins escapes as ordinary searchable text. --- .../editor/markdown-round-trip.test.ts | 20 ++++- .../editor/rich-markdown-inline-math.ts | 2 +- .../rich-markdown-source-reconcile.test.ts | 17 +++++ .../editor/rich-markdown-source-reconcile.ts | 75 +++++++++++-------- 4 files changed, 80 insertions(+), 34 deletions(-) diff --git a/src/renderer/src/components/editor/markdown-round-trip.test.ts b/src/renderer/src/components/editor/markdown-round-trip.test.ts index 1da8315e1f4..ca4d9818e33 100644 --- a/src/renderer/src/components/editor/markdown-round-trip.test.ts +++ b/src/renderer/src/components/editor/markdown-round-trip.test.ts @@ -673,7 +673,25 @@ describe('rich markdown round trip', () => { }) it('does not turn escaped dollars into inline math', () => { - expect(countInlineMathNodes('shell \\$HOME\\$ var and \\$x\\$ too')).toBe(0) + const content = 'shell \\$HOME\\$ var, \\$x\\$ too, and costs $5 to \\$x here' + expect(countInlineMathNodes(content)).toBe(0) + expect(roundTripMarkdown(`${content}\n`)).toBe(content) + }) + + it('keeps escaped characters as searchable text', () => { + const codec = createRichMarkdownEditorCodec() + const editor = new Editor({ + element: null, + extensions: createRichMarkdownExtensions({ codec }), + content: encodeRawMarkdownHtmlForRichEditor('cost \\$1,200 and file\\_name\n', codec), + contentType: 'markdown' + }) + try { + // Why: find/replace treats atoms as read-only, so escapes must stay ordinary text. + expect(editor.state.doc.textContent).toBe('cost $1,200 and file_name') + } finally { + editor.destroy() + } }) it('keeps dollar amounts as text instead of inline math', () => { diff --git a/src/renderer/src/components/editor/rich-markdown-inline-math.ts b/src/renderer/src/components/editor/rich-markdown-inline-math.ts index 05f6d32fda3..bca71c9fb55 100644 --- a/src/renderer/src/components/editor/rich-markdown-inline-math.ts +++ b/src/renderer/src/components/editor/rich-markdown-inline-math.ts @@ -1,7 +1,7 @@ import { InlineMath } from '@tiptap/extension-mathematics' // Keep money as text while allowing valid multiline and escaped LaTeX content. -const INLINE_MATH = /^\$(?![\s$])([^$]*?[^\s$])\$(?![\d$])/ +const INLINE_MATH = /^\$(?![\s$])((?:\\[\s\S]|[^$\\])*?)(? { expect(reconciled).toBe('Cost \\$1 for _em_.\n\ntext\n\nAdded') }) + it('keeps source style when appending to a source whose last line is whitespace only', () => { + // Why: the trailing run is one newline, so the body strip is tried first; it lands after the + // spaces and fails the proof, and the whole-text patch must then take over instead of canonical. + const originalSource = 'Cost \\$1 & co.\n\nLast.\n \n' + const baseCanonical = serialize(originalSource)! + const edited = `${baseCanonical} Added.` + + const reconciled = reconcileSerializedMarkdown({ + originalSource, + baseCanonical, + edited, + roundTrip: (md) => serialize(md) + }) + + expect(reconciled).toBe('Cost \\$1 & co.\n\nLast. Added.\n \n') + }) + it('keeps source style when deleting the trailing empty paragraph', () => { const originalSource = 'Cost \\$1 for _em_.\n\ntext\n\n' const baseCanonical = serialize(originalSource)! diff --git a/src/renderer/src/components/editor/rich-markdown-source-reconcile.ts b/src/renderer/src/components/editor/rich-markdown-source-reconcile.ts index 370988eb2eb..dd2915fad5a 100644 --- a/src/renderer/src/components/editor/rich-markdown-source-reconcile.ts +++ b/src/renderer/src/components/editor/rich-markdown-source-reconcile.ts @@ -30,6 +30,8 @@ export type ReconcileSerializedMarkdownParams = { roundTrip: (markdown: string) => string | null } +type PatchAttempt = { source: string; base: string; edited: string; trailing: string } + export function restoreMarkdownSourceEol(markdown: string, source: string): string { return restoreEol(toLf(markdown), detectDominantEol(source)) } @@ -92,24 +94,47 @@ export function reconcileSerializedMarkdown({ return canonicalFallback() } + // Why: dmp's half-match accelerator ignores the diff deadline (100ms+ on repeated seeds), so bail to canonical for highly repetitive replacements. + if (hasRepeatedHalfMatchSeed(baseLf, editedLf)) { + return canonicalFallback() + } + // Branch 4: run the divergent-base patch entirely in LF space. // Why: when the source ends in one newline that canonical lacks, an end-of-document hunk (whose // trailing context is dmp's end-of-text padding) lands after that newline and fails branch 6, so - // patch the bodies and re-attach the run. Any other shape (a trailing empty paragraph is `\n\n` - // in canonical too) patches correctly whole. - const editedTrailingNewlines = editedLf.match(/\n+$/)?.[0] ?? '' - const stripEnd = !baseLf.endsWith('\n') && originalTrailingNewlines.length === 1 - const sourceBody = stripEnd ? stripTrailingNewlines(originalSourceLf) : originalSourceLf - const baseBody = baseLf - const editedBody = stripEnd ? stripTrailingNewlines(editedLf) : editedLf - const reconciledTrailingNewlines = stripEnd - ? editedTrailingNewlines + originalTrailingNewlines - : '' - // Why: dmp's half-match accelerator ignores the diff deadline (100ms+ on repeated seeds), so bail to canonical for highly repetitive replacements. - if (hasRepeatedHalfMatchSeed(baseBody, editedBody)) { - return canonicalFallback() + // try the bodies first and re-attach the run; the whole-text patch (right for every other shape) + // stays as the second attempt so no file does worse than before. + const attempts: PatchAttempt[] = [] + if (!baseLf.endsWith('\n') && originalTrailingNewlines.length === 1) { + attempts.push({ + source: stripTrailingNewlines(originalSourceLf), + base: baseLf, + edited: stripTrailingNewlines(editedLf), + trailing: (editedLf.match(/\n+$/)?.[0] ?? '') + originalTrailingNewlines + }) } - let diffs = makeDiff(baseBody, editedBody, { + attempts.push({ source: originalSourceLf, base: baseLf, edited: editedLf, trailing: '' }) + + for (const attempt of attempts) { + // Branch 5: a hunk failed to locate in the non-canonical source → unreliable fuzzy match. + const patched = patchDivergentSource(attempt.source, attempt.base, attempt.edited) + if (patched === null) { + continue + } + const reconciledLf = patched + attempt.trailing + // Branch 6: prove reconciled bytes render-equal the editor's document — any fuzzy misplacement changes canonical output and is caught here. + const reparsed = roundTrip(reconciledLf) + if (reparsed !== null && normalizeForSafety(reparsed) === normalizeForSafety(editedLf)) { + // Restore the detected EOL as the final step so reconciled CRLF stays CRLF. + return restoreEol(reconciledLf, eol) + } + } + return canonicalFallback() +} + +/** Applies the base→edited diff onto the divergent source; null when a hunk could not be located. */ +function patchDivergentSource(source: string, base: string, edited: string): string | null { + let diffs = makeDiff(base, edited, { checkLines: true, timeout: RECONCILE_DIFF_TIMEOUT_SECONDS }) @@ -118,32 +143,18 @@ export function reconcileSerializedMarkdown({ diffs = cleanupSemantic(diffs) diffs = cleanupEfficiency(diffs) } - const patches = makePatches(baseBody, diffs) + const patches = makePatches(base, diffs) // Why: applyPatches decodes starts as UTF-8 offsets even though makePatches returns UTF-16 indices; encode against the divergent text being patched so decoding preserves the fuzzy-match seed. const utf8Offsets = getUtf8OffsetsAtCodeUnitIndices( - sourceBody, + source, patches.flatMap((patch) => [patch.start1, patch.start2]) ) for (const patch of patches) { patch.start1 = utf8Offsets.get(patch.start1) ?? 0 patch.start2 = utf8Offsets.get(patch.start2) ?? 0 } - const [reconciledBody, results] = applyPatches(patches, sourceBody) - const reconciledLf = reconciledBody + reconciledTrailingNewlines - - // Branch 5: a hunk failed to locate in the non-canonical source → unreliable fuzzy match, fall back to canonical. - if (results.some((applied) => !applied)) { - return canonicalFallback() - } - - // Branch 6: prove reconciled bytes render-equal the editor's document — any fuzzy misplacement changes canonical output and is caught here → canonical fallback. - const reparsed = roundTrip(reconciledLf) - if (reparsed === null || normalizeForSafety(reparsed) !== normalizeForSafety(editedLf)) { - return canonicalFallback() - } - - // Restore the detected EOL as the final step so reconciled CRLF stays CRLF. - return restoreEol(reconciledLf, eol) + const [patched, results] = applyPatches(patches, source) + return results.some((applied) => !applied) ? null : patched } function stripTrailingNewlines(lfText: string): string {