fix: Tiptap round-trip safety + autosave scoping for #437/#440/#449 (31/31, 8/8 real files)#445
Merged
Merged
Conversation
…-trip Captures 14 failure modes in the Tiptap+tiptap-markdown round-trip used by the file-preview pane (mountMarkdownEditor in editor.ts). The autosave loop in controller.ts reads getTiptapMarkdown() and diffs it against state.fullDocumentContent, then emits edit_block calls for every hunk. Any drift introduced by parse-and-reserialize gets silently written to disk. Failure cases (all 14/14 fail on current main): - GFM pipe table -> 'AB12' - '~' -> '\~' (prosemirror-markdown strikethrough escaping) - Adjacent block elements gain blank-line separators - Wikilink + heading combo drifts on spacing - Trailing newline stripped - Combined #437 fixture - YAML frontmatter --- parsed as Setext heading (#437 LevionLaurion) - '[x]' -> '\[x\]' (#440) - Underscore-rich identifiers drift on trailing newline (#440) - '~/path' -> '\~/path' (#440) - Loose lists drift on trailing newline (#440) - CRLF -> LF + soft-break collapse (related to #97) - Realistic README-style file: tables collapse + soft breaks merge, >70%-changed threshold in computeEditBlocks would emit a single edit_block replacing the entire file with the degraded version - Realistic doc with table embedded in prose: prose around table also lost via soft-break merge Adds jsdom@^24 as a devDependency to mount Tiptap headlessly.
Contributor
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Tiptap table extensions and jsdom dev dependency; implements a preprocess → Tiptap → postprocess round-trip pipeline and exported helpers for markdown edit-diffing; updates editor change API and workspace state to track dirty line ranges; adds two Node/jsdom regression tests; small telemetry key rename. Changes
Sequence Diagram(s)sequenceDiagram
actor Tester
participant Pre as "Preprocess\n(preprocessForEditor)"
participant Editor as "Tiptap Editor\n(buildTiptapExtensions)"
participant Serializer as "Serializer\n(editor.getValue)"
participant Post as "Postprocess\n(applyPostProcess)"
Tester->>Pre: provide input markdown
Pre->>Editor: editorInput (EOL normalized,\nfrontmatter stripped, placeholders)
Editor->>Serializer: user edits / getValue()
Serializer->>Post: serialized markdown
Post->>Tester: final output (EOL restored,\nfrontmatter & placeholders restored,\nserializer fixups applied)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/test-markdown-editor-roundtrip.js (1)
378-378: Top-levelrunAllTests()is fire-and-forget — propagate harness errors explicitly.If
runAllTestsrejects before the per-testtry/catchcan engage (e.g. a global setup error from the imports above, or a programming error in the harness itself), the exit code depends entirely on Node's--unhandled-rejectionsmode, which has shifted across versions. Make the failure mode explicit so the test runner reports a clear non-zero exit:Suggested fix
-runAllTests(); +runAllTests().catch((err) => { + console.error('FATAL: test harness crashed before/after tests completed'); + console.error(err); + process.exit(1); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-markdown-editor-roundtrip.js` at line 378, The top-level call to runAllTests() is fire-and-forget and can leave harness failures as unhandled rejections; change the invocation of runAllTests so rejections are handled explicitly (e.g. attach a .catch handler or await inside an async IIFE) that logs the error (including the error object) and calls process.exit(1) to ensure the process exits non‑zero on failure; update the invocation of runAllTests in test/test-markdown-editor-roundtrip.js accordingly so any rejection from runAllTests is caught, reported, and causes a non-zero exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/test-markdown-editor-roundtrip.js`:
- Around line 211-221: The test function testFrontmatterListItem is misnamed for
its assertion about loose lists; rename testFrontmatterListItem to something
descriptive like testLooseListPreserved or testListWithBlankLinesBetweenItems so
failures clearly indicate the blank-line-between-list-items regression being
tested (you can mirror naming style used by testYamlFrontmatterSurvives for
consistency). Update the function declaration and any references to it in the
test suite so the test runner reports the correct test name.
- Around line 224-239: The test testCrlfPreserved currently asserts the editor
layer preserves CRLF verbatim, which contradicts upstream/TextFileHandler and
the planned editor behavior; update testCrlfPreserved to expect LF-normalization
instead: feed the CRLF input to roundTrip and assert the output equals the input
after replacing "\r\n" with "\n" (i.e., normalized LF form), referencing
roundTrip and the test name so the intent is explicit that CRLF → LF
normalization is deliberate at the editor boundary.
---
Nitpick comments:
In `@test/test-markdown-editor-roundtrip.js`:
- Line 378: The top-level call to runAllTests() is fire-and-forget and can leave
harness failures as unhandled rejections; change the invocation of runAllTests
so rejections are handled explicitly (e.g. attach a .catch handler or await
inside an async IIFE) that logs the error (including the error object) and calls
process.exit(1) to ensure the process exits non‑zero on failure; update the
invocation of runAllTests in test/test-markdown-editor-roundtrip.js accordingly
so any rejection from runAllTests is caught, reported, and causes a non-zero
exit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c059485e-9f6b-4bee-8893-c7917aa88941
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.jsontest/test-markdown-editor-roundtrip.js
Comment on lines
+211
to
+221
| async function testFrontmatterListItem() { | ||
| console.log('\n--- Test: list with blank lines between items (#440) ---'); | ||
| const input = '- first item\n\n- second item\n\n- third item\n'; | ||
| const output = roundTrip(input); | ||
| assert.strictEqual( | ||
| output, | ||
| input, | ||
| 'Blank lines between list items (loose list) must not be stripped' | ||
| ); | ||
| console.log('OK loose list preserved'); | ||
| } |
Contributor
There was a problem hiding this comment.
Function name doesn't match what the test asserts.
testFrontmatterListItem reads as a frontmatter test, but the body is a loose-list (blank-line-between-items) regression. Looks like a copy/paste from testYamlFrontmatterSurvives. Rename for accurate failure diagnostics.
Suggested rename
-async function testFrontmatterListItem() {
+async function testLooseListBlankLinesPreserved() {
console.log('\n--- Test: list with blank lines between items (`#440`) ---');
@@
- testFrontmatterListItem,
+ testLooseListBlankLinesPreserved,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/test-markdown-editor-roundtrip.js` around lines 211 - 221, The test
function testFrontmatterListItem is misnamed for its assertion about loose
lists; rename testFrontmatterListItem to something descriptive like
testLooseListPreserved or testListWithBlankLinesBetweenItems so failures clearly
indicate the blank-line-between-list-items regression being tested (you can
mirror naming style used by testYamlFrontmatterSurvives for consistency). Update
the function declaration and any references to it in the test suite so the test
runner reports the correct test name.
Comment on lines
+224
to
+239
| async function testCrlfPreserved() { | ||
| console.log('\n--- Test: CRLF line endings preserved (related to #97/#438) ---'); | ||
| // The Tiptap pipeline operates on strings; the file-preview UI seeds itself | ||
| // with content from read_file's text response, which is already LF-normalized | ||
| // by TextFileHandler (PR #438 fixes that upstream). But if a CRLF file | ||
| // somehow reaches this layer with CRLF intact, the round-trip should not | ||
| // silently downgrade to LF. | ||
| const input = '# Heading\r\nFirst line.\r\nSecond line.\r\n'; | ||
| const output = roundTrip(input); | ||
| assert.strictEqual( | ||
| output, | ||
| input, | ||
| 'CRLF line endings must not be silently converted to LF on round-trip' | ||
| ); | ||
| console.log('OK CRLF preserved'); | ||
| } |
Contributor
There was a problem hiding this comment.
CRLF assertion contradicts the documented post-fix behavior.
The inline comment correctly notes that the upstream pipeline (PR #438 in TextFileHandler) normalizes CRLF → LF before content reaches this layer, and the planned CodeMirror successor (#442) also normalizes to LF. Yet the assertion requires output === input with CRLF preserved verbatim. That means this test will continue to fail after #442 lands, even though the fix is working as designed — turning a regression test into a permanent red.
Consider one of:
- Drop the test (CRLF preservation is explicitly not an invariant of the editor layer).
- Invert the assertion: assert that CRLF input gets normalized to an LF output (documenting the deliberate behavior, not flagging it as corruption).
- Move CRLF preservation guarantees to the file I/O layer (
TextFileHandler) where they actually belong.
Suggested change: assert deliberate LF normalization
async function testCrlfPreserved() {
- console.log('\n--- Test: CRLF line endings preserved (related to `#97/`#438) ---');
+ console.log('\n--- Test: CRLF input is deterministically normalized to LF (related to `#97/`#438) ---');
// The Tiptap pipeline operates on strings; the file-preview UI seeds itself
// with content from read_file's text response, which is already LF-normalized
- // by TextFileHandler (PR `#438` fixes that upstream). But if a CRLF file
- // somehow reaches this layer with CRLF intact, the round-trip should not
- // silently downgrade to LF.
+ // by TextFileHandler (PR `#438` fixes that upstream). The editor layer is
+ // documented as LF-only; CRLF preservation is an I/O-layer concern.
const input = '# Heading\r\nFirst line.\r\nSecond line.\r\n';
const output = roundTrip(input);
+ const expected = input.replace(/\r\n/g, '\n');
assert.strictEqual(
output,
- input,
- 'CRLF line endings must not be silently converted to LF on round-trip'
+ expected,
+ 'CRLF must be deterministically normalized to LF (not corrupted in other ways)'
);
- console.log('OK CRLF preserved');
+ console.log('OK CRLF deterministically normalized to LF');
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/test-markdown-editor-roundtrip.js` around lines 224 - 239, The test
testCrlfPreserved currently asserts the editor layer preserves CRLF verbatim,
which contradicts upstream/TextFileHandler and the planned editor behavior;
update testCrlfPreserved to expect LF-normalization instead: feed the CRLF input
to roundTrip and assert the output equals the input after replacing "\r\n" with
"\n" (i.e., normalized LF form), referencing roundTrip and the test name so the
intent is explicit that CRLF → LF normalization is deliberate at the editor
boundary.
Contributor
|
CodeAnt AI finished reviewing your PR. |
Tiptap's parse-and-reserialise round-trip silently mutates user files (see #437, #440). Pure round-trip safety (untouched file -> identical output) is too strict; what matters in production is that an edit produces only the user's actual change, not collateral normalisation. Two test suites: - test-markdown-editor-roundtrip.js (existing, 14 cases): strict round-trip — files survive open->getMarkdown() byte-for-byte. - test-markdown-editor-edit-diff.js (new, 7 cases): realistic — apply a small edit, run computeEditBlocks against the original, assert the diff is bounded (no whole-file rewrite, <=3 hunks, <=20% lines differing, expected text actually present). Implementation: - Centralise Tiptap config in buildTiptapExtensions() so test and production exercise the same code path. Disable strike to stop \~ escaping. Add Table / TableRow / TableHeader / TableCell extensions so GFM pipe tables don't collapse to concatenated cell text. - preprocessForEditor / applyPostProcess wrap the editor with a RoundTripContext that captures: original EOL (LF vs CRLF), YAML frontmatter prefix, gap between frontmatter and body, trailing newline. All re-applied on the way out. - Post-process repairs: unescapeSafeChars (\[, \], \~ in prose), restoreTableSeparatorStyle (|---| vs | --- |), restoreSoftBreaks (Tiptap's space-joined paragraph lines back to original line breaks), collapseBlockSeparators (Tiptap's spurious blank lines between adjacent block elements). Order matters: softBreaks must run before blockSeparators because blockSeparators matches surrounding lines against pairs from the original. - Export computeEditBlocks from controller.ts so the edit-diff test exercises the real autosave decision logic, including the >70% whole-file rewrite trigger. All 21 tests pass. Includes the in-the-wild #437 README fixture, the mixed table+prose+frontmatter case, CRLF preservation, wikilink round-trip, and the realistic 'edit a paragraph in a doc with frontmatter + wikilinks + tasks + table' edit-diff case.
Captured from /Users/eduardsruzga/work/best-value-ai/README.md, which
hit 5 distinct corruption hunks on no-edit open in the previous state.
Now: byte-exact round-trip, 0 hunks.
Tests added (test-markdown-editor-roundtrip.js):
testBareUrlNotAutoLinked — 'https://x' stays bare, no <…>
testEmojiPrefixedSoftBreaksRestored — 3 paragraph lines stay 3 lines
testLinkInTableCellSurvivesRoundTrip — [`x`](url) keeps its href
testStarBulletMarkerPreserved — '*' bullets stay '*', not '-'
Fixes:
- linkify: false. The auto-bracketing ('https://x' -> '<https://x>') was
a serializer-side artifact of the parser's URL-detection rule. Disable
the rule; pasted URLs in the editor still become clickable via
Tiptap's built-in Link extension.
- restoreSoftBreaks now tries both ' ' (the common joiner) and '' (when
the boundary is between punctuation like ')' and a non-letter like an
emoji — the case that broke the 3-line emoji-prefixed sequence in
best-value-ai's README).
- restoreBulletMarkers maps output bullet lines onto source bullet lines
positionally, restoring the marker style ('*' / '-' / '+'). New
bullets the user adds keep the editor's default '-'.
- Code-text links ([`x`](url)) are stripped of their URL by
tiptap-markdown's parser when the link text is purely inline code.
Replace with ASCII placeholders during preprocess; restore in
postprocess. Pattern matches the existing wikilink workaround.
- unescapeSafeChars is now line-aligned: it only removes \[, \], \~
escapes when the same source line, stripped of those escapes, also
matches the output line stripped. Means we never touch escapes the
user authored — we only undo escapes Tiptap added.
Also wired the codeLinks placeholder mapping into the RoundTripContext
so production autosave gets the same restore behaviour as the test
harness.
Result: 18/18 strict + 7/7 edit-diff = 25/25 passing. The
best-value-ai/README.md (12977 bytes, 214 lines) round-trips byte-for-
byte with 0 autosave hunks.
wonderwhy-er
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/file-preview/src/markdown/editor.ts`:
- Around line 243-267: The restoreBulletMarkers function currently applies
markers by position (origMarkers + bulletIdx) which breaks when items are
inserted/removed; change the algorithm to match bullets by their stripped
content instead: build a map from stripped original bullet text (use the same
BULLET_RE to split) to a FIFO queue of original markers (origMarkersPerText),
then when iterating outLines use the stripped text (outText =
outLines[i].slice(m[0].length)) to dequeue the next marker for that text and
apply it only if the dequeued marker exists and differs from m[2]; remove the
positional bulletIdx logic and ensure consumed markers are tracked so duplicates
and re-ordered bullets get the correct original marker for identical texts.
- Around line 699-707: The code captures a single "context" at mount via
preprocessForEditor(options.value) and then always uses that frozen context in
getTiptapMarkdown/applyPostProcess, causing setValue to push raw content that
later gets post-processed with stale metadata; fix by recomputing
preprocessForEditor for the new value inside setValue (or updating the stored
context whenever programmatic updates occur), then push the properly
preprocessed editorInput into Tiptap and update the module-level context used by
getTiptapMarkdown so applyPostProcess uses the current document's
frontmatter/EOL/trailing-newline/code-link state (refer to preprocessForEditor,
setValue, getTiptapMarkdown, applyPostProcess).
- Around line 465-478: The Strike extension was disabled in
buildTiptapExtensions via StarterKit.configure({ strike: false }) but the UI
still renders a Strike button and handleFormatClick still calls
tiptap.chain().focus().toggleStrike().run(); remove the Strike button from the
toolbar rendering and delete the corresponding handler branch in
handleFormatClick (the toggleStrike() call) so no UI references the disabled
extension; alternatively, if you prefer to keep strike support, enable the
strike extension in buildTiptapExtensions instead of removing UI/handler—update
either buildTiptapExtensions (StarterKit.configure) or the toolbar rendering and
handleFormatClick accordingly to make them consistent.
In `@test/test-markdown-editor-edit-diff.js`:
- Around line 134-138: The test harness in assertEditDiffIsClean should catch
exceptions from edit(handle) and handle.getMarkdown() so a thrown error calls
fail(...) but still runs cleanup and allows other tests to continue; update
assertEditDiffIsClean to wrap edit(handle) and the getMarkdown() call in
try/catch blocks (or a single try with separate error handling), call
fail(error) inside the catch instead of letting the exception escape, and ensure
handle.destroy() is invoked in a finally block so cleanup always runs; reference
the existing function assertEditDiffIsClean and its use of mountForEdit,
edit(handle), handle.getMarkdown(), and handle.destroy() when applying this
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05ac3a4f-9ad5-49a4-ac24-f60ae088e651
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.jsonsrc/ui/file-preview/src/markdown/controller.tssrc/ui/file-preview/src/markdown/editor.tstest/test-markdown-editor-edit-diff.jstest/test-markdown-editor-roundtrip.js
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
Comment on lines
+243
to
+267
| function restoreBulletMarkers(serialized: string, originalInput: string): string { | ||
| const BULLET_RE = /^(\s*)([*\-+])(\s)/; | ||
| const origLines = originalInput.replace(/\r\n/g, '\n').split('\n'); | ||
| // Collect markers in source order. We index purely by position in | ||
| // the bullet sequence — no attempt to match by content, so re-ordered | ||
| // bullets still get sensible markers. | ||
| const origMarkers: string[] = []; | ||
| for (const line of origLines) { | ||
| const m = line.match(BULLET_RE); | ||
| if (m) origMarkers.push(m[2]); | ||
| } | ||
| if (origMarkers.length === 0) return serialized; | ||
|
|
||
| const outLines = serialized.split('\n'); | ||
| let bulletIdx = 0; | ||
| for (let i = 0; i < outLines.length; i += 1) { | ||
| const m = outLines[i].match(BULLET_RE); | ||
| if (!m) continue; | ||
| const wanted = origMarkers[bulletIdx]; | ||
| if (wanted && wanted !== m[2]) { | ||
| outLines[i] = m[1] + wanted + m[3] + outLines[i].slice(m[0].length); | ||
| } | ||
| bulletIdx += 1; | ||
| } | ||
| return outLines.join('\n'); |
Contributor
There was a problem hiding this comment.
Ordinal bullet restoration rewrites the wrong items after prepends or mid-list inserts.
This logic matches markers by sequence index, not by bullet identity. If a user inserts a new item before existing bullets, the new item consumes the first saved marker and a later untouched bullet falls back to -, so autosave rewrites lines the user never changed. Match bullets by content/LCS, or only restore a marker when the stripped bullet text still matches the original line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/file-preview/src/markdown/editor.ts` around lines 243 - 267, The
restoreBulletMarkers function currently applies markers by position (origMarkers
+ bulletIdx) which breaks when items are inserted/removed; change the algorithm
to match bullets by their stripped content instead: build a map from stripped
original bullet text (use the same BULLET_RE to split) to a FIFO queue of
original markers (origMarkersPerText), then when iterating outLines use the
stripped text (outText = outLines[i].slice(m[0].length)) to dequeue the next
marker for that text and apply it only if the dequeued marker exists and differs
from m[2]; remove the positional bulletIdx logic and ensure consumed markers are
tracked so duplicates and re-ordered bullets get the correct original marker for
identical texts.
Comment on lines
+465
to
+478
| export function buildTiptapExtensions(): Extensions { | ||
| return [ | ||
| StarterKit.configure({ | ||
| heading: { levels: [1, 2, 3, 4, 5, 6] }, | ||
| codeBlock: { HTMLAttributes: { class: 'code-viewer' } }, | ||
| link: { | ||
| openOnClick: false, | ||
| autolink: true, | ||
| HTMLAttributes: { 'data-markdown-link': 'true' }, | ||
| }, | ||
| // Disable strikethrough — see comment above. The serializer | ||
| // would otherwise treat `~` as a strike delimiter character | ||
| // and emit `\~` to escape it. | ||
| strike: false, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file exists and get its total lines
wc -l src/ui/file-preview/src/markdown/editor.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 124
🏁 Script executed:
# Read lines 465-478 to confirm the strike: false configuration
sed -n '465,478p' src/ui/file-preview/src/markdown/editor.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 681
🏁 Script executed:
# Find the renderFormattingButtons function to check if strike button is still present
rg -n "renderFormattingButtons" src/ui/file-preview/src/markdown/editor.ts -A 30Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 5552
🏁 Script executed:
# Find the handleFormatClick function to check if toggleStrike is still called
rg -n "handleFormatClick" src/ui/file-preview/src/markdown/editor.ts -A 30Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 4534
🏁 Script executed:
# Search for all occurrences of "strike" in the file to get a complete picture
rg -n "strike" src/ui/file-preview/src/markdown/editor.ts -iRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 1029
🏁 Script executed:
# Find package.json to check Tiptap version
fd package.jsonRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 89
🏁 Script executed:
# Check the Tiptap version in the relevant package file
cat package.json | grep -A 2 -B 2 "tiptap"Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 678
🏁 Script executed:
# Look for any additional context about the strike extension or comments
rg -n "tiptap" src/ui/file-preview/src/markdown/editor.ts | head -20Repository: wonderwhy-er/DesktopCommanderMCP
Length of output: 1351
🏁 Script executed:
# Check if there are any comments around line 463 that explain the decision
sed -n '450,480p' src/ui/file-preview/src/markdown/editor.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 1434
Remove the Strike button and handler—the extension is disabled but the UI still references it.
The strike extension is disabled at line 478 (strike: false in StarterKit.configure()), which removes the toggleStrike() command from Tiptap v3.22.3. However, the formatting toolbar at line 559 still renders the Strike button, and handleFormatClick at lines 995–996 still attempts to call tiptap.chain().focus().toggleStrike().run(), causing a runtime error or silent failure when the button is clicked. The code comment claims "the editor never offered a strike button," but the HTML contradicts this. Either enable the strike extension or remove the button and handler.
Fix directions
Remove the button from the toolbar:
<button class="markdown-format-button" type="button" data-format="bold"><strong>B</strong></button>
<button class="markdown-format-button" type="button" data-format="italic"><em>I</em></button>
- <button class="markdown-format-button" type="button" data-format="strike"><span style="text-decoration:line-through">S</span></button>
<span class="markdown-format-sep" aria-hidden="true"></span>Remove the handler case:
case 'italic':
tiptap.chain().focus().toggleItalic().run();
break;
- case 'strike':
- tiptap.chain().focus().toggleStrike().run();
- break;
case 'quote':🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/file-preview/src/markdown/editor.ts` around lines 465 - 478, The
Strike extension was disabled in buildTiptapExtensions via
StarterKit.configure({ strike: false }) but the UI still renders a Strike button
and handleFormatClick still calls tiptap.chain().focus().toggleStrike().run();
remove the Strike button from the toolbar rendering and delete the corresponding
handler branch in handleFormatClick (the toggleStrike() call) so no UI
references the disabled extension; alternatively, if you prefer to keep strike
support, enable the strike extension in buildTiptapExtensions instead of
removing UI/handler—update either buildTiptapExtensions (StarterKit.configure)
or the toolbar rendering and handleFormatClick accordingly to make them
consistent.
Comment on lines
+134
to
+138
| function assertEditDiffIsClean(name, input, edit, expectedSubstring) { | ||
| const handle = mountForEdit(input); | ||
| edit(handle); | ||
| const after = handle.getMarkdown(); | ||
| handle.destroy(); |
Contributor
There was a problem hiding this comment.
Keep the harness running when an edit helper throws.
edit(handle) or handle.getMarkdown() exceptions bypass fail(...), skip cleanup, and abort the rest of the cases. That turns one regression into an uncaught exception instead of a full failure report.
Proposed fix
function assertEditDiffIsClean(name, input, edit, expectedSubstring) {
const handle = mountForEdit(input);
- edit(handle);
- const after = handle.getMarkdown();
- handle.destroy();
+ let after;
+ try {
+ edit(handle);
+ after = handle.getMarkdown();
+ } catch (err) {
+ fail(name, err instanceof Error ? err.message : String(err));
+ return;
+ } finally {
+ handle.destroy();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function assertEditDiffIsClean(name, input, edit, expectedSubstring) { | |
| const handle = mountForEdit(input); | |
| edit(handle); | |
| const after = handle.getMarkdown(); | |
| handle.destroy(); | |
| function assertEditDiffIsClean(name, input, edit, expectedSubstring) { | |
| const handle = mountForEdit(input); | |
| let after; | |
| try { | |
| edit(handle); | |
| after = handle.getMarkdown(); | |
| } catch (err) { | |
| fail(name, err instanceof Error ? err.message : String(err)); | |
| return; | |
| } finally { | |
| handle.destroy(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/test-markdown-editor-edit-diff.js` around lines 134 - 138, The test
harness in assertEditDiffIsClean should catch exceptions from edit(handle) and
handle.getMarkdown() so a thrown error calls fail(...) but still runs cleanup
and allows other tests to continue; update assertEditDiffIsClean to wrap
edit(handle) and the getMarkdown() call in try/catch blocks (or a single try
with separate error handling), call fail(error) inside the catch instead of
letting the exception escape, and ensure handle.destroy() is invoked in a
finally block so cleanup always runs; reference the existing function
assertEditDiffIsClean and its use of mountForEdit, edit(handle),
handle.getMarkdown(), and handle.destroy() when applying this change.
…arkdown-roundtrip
Tiptap's Link extension validates URLs and silently drops links whose
URL doesn't match its scheme/relative-prefix allow-list. Bare relative
paths with subdirectories (`scripts/foo.mjs`, `references/output.md`)
fall through this validation — the parse drops the link, leaving just
the link text.
Most common corruption mode in real skill files (SKILL.md routinely
links to `scripts/` and `references/`).
Generalised the existing code-text-link placeholder workaround into
INLINE_LINK_RE + isFragileLink(). The same regex and placeholder system
now handles both:
1. Code-text links: `[\`x\`](url)`
2. Bare-relative-subpath links: `[X](dir/file)`
URLs we leave alone (Tiptap accepts them):
- Schemes (http://, mailto:, tel:, ftp:, etc.)
- Anchors (#section)
- Single-segment paths (file.md)
- Explicitly-relative paths (./, ../, /)
Tests:
- testRelativePathLinksSurvive captures the failure with skill-files
fixtures (init-skill.mjs, validate-skill.mjs, references/output.md,
section anchors).
- All 19 strict + 7 edit-diff tests still pass.
Skill-files batch test: 4 of 8 SKILL.md files now round-trip byte-exact
(was 1). Remaining failures are different bugs: HTML entity escaping,
trailing-whitespace strip, bold-around-inline-code mis-shifting.
Captured by running the SKILL.md files in ~/.desktop-commander/skills/
through the round-trip pipeline. 4 of 8 files broke before this
commit; all 8 now round-trip byte-exact.
Tests added (test-markdown-editor-roundtrip.js):
testLessThanInProseNotEscaped '< $0.01' -> '< $0.01'
testTrailingHardBreakWhitespacePreserved 'foo \n' (CommonMark hard
break) lost the trailing
spaces or rewritten as
'foo\\\n'
testBoldAroundInlineCodePreserved '**`x`**' -> '`x`',
'**`x` + `y`**' ->
'`x` **+** `y`',
'**Key in `x`:**' ->
'**Key in** `x`**:**'
testEscapedPipeInTableCellPreserved '\\|' inside a cell becomes
bare '|', splits the cell
testListItemWithContinuationLine '- item\n cont\n' joins
to '- item cont\n'
Fixes:
- unescapeHtmlEntitiesInProse: undo Tiptap's '<'/'>'/'&' HTML
entity escaping in prose. Line-aligned like unescapeSafeChars; only
removes entities when the corresponding source line, stripped of
those entities, matches the output line. User-authored entities are
preserved.
- restoreTrailingHardBreaks: detect source lines ending in two
trailing spaces and re-add them. Handles both Tiptap serializer
shapes: '\\\n' line continuation (paragraph) and silently-stripped
(list item).
- BOLD_AROUND_CODE_RE + boldCodeRuns: placeholder '**…`code`…**'
spans during preprocess, restore after serialize. Bypasses
ProseMirror's flat-mark schema limitation.
- pipeEscapeCount + PIPE_ESCAPE_TOKEN: placeholder '\|' as an ASCII
token before parse. Tiptap's serializer drops the backslash; the
token round-trips intact.
- restoreSoftBreaks: recognise list-item-with-lazy-continuation pairs
(list-header followed by 2-space-indented prose line) and try the
de-indented form of the continuation when looking for Tiptap's
joined output.
Result: 24/24 strict + 7/7 edit-diff = 31/31. All 8 skill files in
the stress test now byte-exact.
The capture_call_tool send-site was emitting 'name' as the tool-name property, which collides with GA4's reserved 'name' parameter when passed verbatim. Rename to 'tool_name' at the call site (server.ts) and have capture_call_tool's high-volume routing read either field, falling back to 'name' for backwards compat with any in-flight callers. No behaviour change; pure rename.
Brings in two complementary safety layers from test/issue-437-plus-449-markdown-save: a66e9f6 fix(file-preview): prevent unnecessary markdown autosaves a23ae76 fix(file-preview): scope markdown autosave edits These work alongside the round-trip-exact parser wrappers in this branch: - Round-trip exactness (this branch): preprocess/postprocess wrappers around Tiptap so that getMarkdown(input) === input byte-for-byte. Eliminates Tiptap normalization drift at the source. - hasUserEdited guard (a66e9f6): onUpdate no-ops until the user has actually typed/pasted/cut/dropped or interacted with a toolbar action. Prevents autosave from firing on initial parse-and-mount. - Edit-range scoping (a23ae76): tracks which line ranges the user actually edited and filters autosave hunks to only those that intersect the edited ranges. Even if Tiptap's parse drifts a few unrelated lines, only the user's actual edit lands on disk. Three layers of defense, each catches a different failure mode: - User opens-and-closes: hasUserEdited prevents the parse->autosave pipeline from firing at all. - User edits one line, Tiptap drifts elsewhere: edit-range scope drops the unrelated drift hunks. - User edits and Tiptap doesn't drift: round-trip exactness means the diff is just the actual edit anyway. Auto-merge succeeded with no conflicts. All tests still pass after merge: test-markdown-editor-roundtrip.js 24/24 strict round-trip test-markdown-editor-edit-diff.js 7/7 realistic edit-diff ~/.desktop-commander/skills/*/SKILL.md 8/8 byte-exact stress test
wonderwhy-er
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/file-preview/src/markdown/controller.ts (1)
224-291:⚠️ Potential issue | 🟠 MajorEqual-slice fallback in
splitOversizedEditBlockcan produce non-unique or mis-paired chunks.When
computeEditBlocksproduces a single hunk covering the whole document (line 286), this fallback chopsoldText/newTextintoblockCountequal-line slices and pairs them positionally. Three issues with that:
- A chunk's
old_stringis just an arbitrary range of lines from the original; it can occur multiple times in the document (e.g., repeated empty paragraphs, recurring section headers).edit_blockis invoked withexpected_replacements: 1(line 829), so any slice that isn't unique on disk will be skipped.- Since slicing is independent for old and new (different
oldLines.lengthvsnewLines.lengthdistributions), a "chunk pair" doesn't necessarily correspond to a meaningful semantic edit — the n-th old slice and n-th new slice can describe overlapping regions of the same change.- In
applyEditBlocksToTextthe sequentialcurrent.replace(block.old_string, block.new_string)uses first-occurrence semantics; if an earlier block'snew_stringintroduces text matching a later block'sold_string, the later replace targets the wrong location andstate.fullDocumentContentdiverges from what was actually written to disk.The save error path (lines 842-855) does mitigate the user-visible blast radius, but the local
statebaseline can silently desync when the local-apply succeeds while the remoteedit_blockcall reports a different post-state. Consider using a content-anchored split (e.g., split at lines that are unique inoldTextand shared withnewText) so each chunk'sold_stringis locatable, or skip the split entirely and let the loop fall through to a single hunk with the surrounding error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/file-preview/src/markdown/controller.ts` around lines 224 - 291, splitOversizedEditBlock currently does equal-line positional slicing which can create non-unique or mis-paired chunk pairs and cause applyEditBlocksToText/edit_block mismatches; update computeEditBlocks/splitOversizedEditBlock so that when a full-document single hunk fallback would trigger you instead either (A) perform a content-anchored split: find safe split points (lines that are unique in oldText and also present in newText) and cut blocks at those anchors so each block.old_string is locatable, pairing the surrounding new lines accordingly, or (B) if no safe anchors are found, do not split at all and return one block (oldText/newText) to let the existing remote error handling run; ensure the change touches splitOversizedEditBlock and the computeEditBlocks path that calls it and preserves semantics expected by applyEditBlocksToText and edit_block (including expected_replacements behavior).
🧹 Nitpick comments (2)
src/ui/file-preview/src/markdown/controller.ts (1)
1050-1066: Verify thatonChange's fallback range doesn't paper over a Tiptap regression.When
editRangesis empty/undefined andstate.dirtybecomes true, the fallback[{ fromLine: 1, toLine: value.split('\n').length }]covers the entire document, which effectively disables the range-filter optimization for that change. This is the correct safe default, but ifcomputeSerializedEditRangesever returns[]for a meaningful edit (e.g., due to a bug in the LCS path or a future refactor), every save will silently fall back to whole-document diffing without surfacing the regression. Consider asserting/logging in dev wheneditRangesis unexpectedly empty for adirtytransition, so a regression in the editor's diff-range emission is observable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/file-preview/src/markdown/controller.ts` around lines 1050 - 1066, In the onChange handler (the function that updates state.draftContent, state.dirty and state.dirtyLineRanges), add a dev-only assertion/log when editRanges is null/undefined or an empty array but state.dirty just became true so we are about to fallback to [{ fromLine: 1, toLine: value.split('\n').length }]; specifically, detect the case after computing nextRanges but before assigning state.dirtyLineRanges and emit a non-fatal warning (e.g., console.warn or process/dev logger) that computeSerializedEditRanges (or the editor diff path that produces editRanges) returned an empty set for a meaningful edit so reviewers can spot regressions, and guard this log behind a development check so production behavior remains the same.src/ui/file-preview/src/markdown/editor.ts (1)
113-122:isFragileLinkdoesn't handle inline code wrapped in additional formatting or links with extended text.Two fragile shapes that may slip through:
[followed by inline code with surrounding characters:[**\x`**](url)or`x` more` — text isn't purely inline code, so the regex at line 115 returns false, but tiptap-markdown still drops the URL when the link text starts/ends with code.- Bare-relative URLs that begin with a non-
/character but aren't paths: e.g.mailtotypo without colon, or fragment-only links like?query=foo.Consider broadening the code-link check to "text contains inline code as the only non-whitespace content" and adding a small allow-list/deny-list around URL shapes you've actually validated. Not a blocker for this PR — current tests pass — but worth a follow-up if more fragile shapes surface in the wild.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/file-preview/src/markdown/editor.ts` around lines 113 - 122, The isFragileLink function currently only flags text that is exactly backtick-wrapped (regex in isFragileLink) and treats some URLs as fragile; update isFragileLink so the text check detects inline code as the sole non-whitespace content even if wrapped by other formatting characters (e.g. bold/italic or trailing/leading words) by trimming whitespace and stripping common formatting wrappers before testing for a backtick-delimited code span (refer to isFragileLink for the check); additionally introduce a small URL allow/deny sanity check inside isFragileLink that treats bare fragments/query-only URLs (startsWith('?') or startsWith('#')) and scheme-typos (e.g. alphanumeric sequences without a colon) as non-schemed/fragile while allowing true schemes (use hasScheme = /^[a-z][a-z0-9+.-]*:/i.test(url)) and explicitly exclude obvious false positives like mailto-typos by checking for ':' presence after a known scheme token set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/file-preview/src/markdown/editor.ts`:
- Line 91: The placeholder tokens (PIPE_ESCAPE_TOKEN, TIPTAPCODELINK%04d,
TIPTAPBOLDCODE%04d) can collide with literal document content and get
incorrectly rewritten in applyPostProcess; update the placeholder generation
logic used before parsing to first check the input for any existing occurrences
of the chosen token names and if a collision is detected either (a) switch to a
unique per-document token by appending a short hash/nonce derived from the
document content to PIPE_ESCAPE_TOKEN and the TIPTAP... placeholders, or (b)
skip the placeholder strategy for that document (fail-safe path), and ensure
applyPostProcess uses the same generated token values when restoring originals;
locate and change the code that defines PIPE_ESCAPE_TOKEN and the code that
emits TIPTAPCODELINK/TIPTAPBOLDCODE as well as applyPostProcess so they share
the collision-checked/hashed token generation.
---
Outside diff comments:
In `@src/ui/file-preview/src/markdown/controller.ts`:
- Around line 224-291: splitOversizedEditBlock currently does equal-line
positional slicing which can create non-unique or mis-paired chunk pairs and
cause applyEditBlocksToText/edit_block mismatches; update
computeEditBlocks/splitOversizedEditBlock so that when a full-document single
hunk fallback would trigger you instead either (A) perform a content-anchored
split: find safe split points (lines that are unique in oldText and also present
in newText) and cut blocks at those anchors so each block.old_string is
locatable, pairing the surrounding new lines accordingly, or (B) if no safe
anchors are found, do not split at all and return one block (oldText/newText) to
let the existing remote error handling run; ensure the change touches
splitOversizedEditBlock and the computeEditBlocks path that calls it and
preserves semantics expected by applyEditBlocksToText and edit_block (including
expected_replacements behavior).
---
Nitpick comments:
In `@src/ui/file-preview/src/markdown/controller.ts`:
- Around line 1050-1066: In the onChange handler (the function that updates
state.draftContent, state.dirty and state.dirtyLineRanges), add a dev-only
assertion/log when editRanges is null/undefined or an empty array but
state.dirty just became true so we are about to fallback to [{ fromLine: 1,
toLine: value.split('\n').length }]; specifically, detect the case after
computing nextRanges but before assigning state.dirtyLineRanges and emit a
non-fatal warning (e.g., console.warn or process/dev logger) that
computeSerializedEditRanges (or the editor diff path that produces editRanges)
returned an empty set for a meaningful edit so reviewers can spot regressions,
and guard this log behind a development check so production behavior remains the
same.
In `@src/ui/file-preview/src/markdown/editor.ts`:
- Around line 113-122: The isFragileLink function currently only flags text that
is exactly backtick-wrapped (regex in isFragileLink) and treats some URLs as
fragile; update isFragileLink so the text check detects inline code as the sole
non-whitespace content even if wrapped by other formatting characters (e.g.
bold/italic or trailing/leading words) by trimming whitespace and stripping
common formatting wrappers before testing for a backtick-delimited code span
(refer to isFragileLink for the check); additionally introduce a small URL
allow/deny sanity check inside isFragileLink that treats bare
fragments/query-only URLs (startsWith('?') or startsWith('#')) and scheme-typos
(e.g. alphanumeric sequences without a colon) as non-schemed/fragile while
allowing true schemes (use hasScheme = /^[a-z][a-z0-9+.-]*:/i.test(url)) and
explicitly exclude obvious false positives like mailto-typos by checking for ':'
presence after a known scheme token set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cde1330d-bd36-4eca-86eb-63c4471994aa
📒 Files selected for processing (3)
src/ui/file-preview/src/markdown/controller.tssrc/ui/file-preview/src/markdown/editor.tssrc/ui/file-preview/src/model.ts
| // Token used to placeholder `\|` escapes. Chosen so it's: | ||
| // - ASCII letters/digits only (survives Tiptap's parse/serialize round trip) | ||
| // - distinctive enough to never collide with real document content | ||
| const PIPE_ESCAPE_TOKEN = 'TIPTAPPIPEESCX'; |
Contributor
There was a problem hiding this comment.
Placeholder tokens can collide with literal document content.
PIPE_ESCAPE_TOKEN = 'TIPTAPPIPEESCX', TIPTAPCODELINK%04d, and TIPTAPBOLDCODE%04d are inserted into the document before Tiptap parsing and split/join'd back out in applyPostProcess regardless of where they appear in the serialized output. If the source already contains any of these literal strings (e.g., a developer doc that references the token in code or prose), the corresponding applyPostProcess step will silently rewrite that user content to \|, the original link, or the bold-code run.
Cheap hardening: detect collisions before assigning placeholders and either (a) bail out of the placeholder strategy for that document, or (b) include a uniqueness suffix derived from the input (e.g., a short hash) so the token cannot pre-exist. Same fix applies to the code-link and bold-code placeholders.
Also applies to: 153-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/file-preview/src/markdown/editor.ts` at line 91, The placeholder
tokens (PIPE_ESCAPE_TOKEN, TIPTAPCODELINK%04d, TIPTAPBOLDCODE%04d) can collide
with literal document content and get incorrectly rewritten in applyPostProcess;
update the placeholder generation logic used before parsing to first check the
input for any existing occurrences of the chosen token names and if a collision
is detected either (a) switch to a unique per-document token by appending a
short hash/nonce derived from the document content to PIPE_ESCAPE_TOKEN and the
TIPTAP... placeholders, or (b) skip the placeholder strategy for that document
(fail-safe path), and ensure applyPostProcess uses the same generated token
values when restoring originals; locate and change the code that defines
PIPE_ESCAPE_TOKEN and the code that emits TIPTAPCODELINK/TIPTAPBOLDCODE as well
as applyPostProcess so they share the collision-checked/hashed token generation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
A combined fix for #437 / #440 / #449 that keeps Tiptap as the editor. An alternative path to #442 (which replaces Tiptap with CodeMirror).
This branch combines two streams of work:
getMarkdown(input) === inputbyte-for-byte. Eliminates Tiptap normalization drift at the source.test/issue-437-plus-449-markdown-save): two safety layers that limit autosave reach.The two streams are complementary — defense in depth, not redundant.
Test results
31/31 passing in unit tests, plus a real-world stress test:
Real-world files round-tripping byte-exact through autosave, zero hunks emitted:
~/work/best-value-ai/README.md(12977 bytes, 214 lines)bigquery-cli/SKILL.mddesktop-commander-guide/SKILL.mdimage-editor/SKILL.mdmedium-article-writer/SKILL.mdreplicate-api/SKILL.mdsentry-posthog-replay-triage/SKILL.mdskill-creator/SKILL.mdviral-shorts-transcript-engine/SKILL.mdThree layers of defense
editor.tspre/post-process wrappersgetMarkdown(input) === inputbyte-for-bytehasUserEditedguardeditor.ts(a66e9f6)onUpdateno-ops until user types/pastes/cuts/drops or uses toolbarcontroller.ts(a23ae76) +editor.tsEach layer catches a different failure mode:
hasUserEditedprevents the parse→autosave pipeline from firing at all.How the tests work
The strict suite (
test-markdown-editor-roundtrip.js) assertsgetMarkdown(input) === inputbyte-for-byte for 24 reproducer fixtures.The edit-diff suite (
test-markdown-editor-edit-diff.js) is the realistic one — it asks: after a small user edit, doescomputeEditBlocks(the actual autosave decision logic incontroller.ts) produce a small, bounded diff? It refuses whole-file rewrites (≥70% lines changed → catastrophic), >3 hunks (Tiptap is normalising unrelated regions), or >20% lines differing.Round-trip approach
Tiptap's parse + serialize round-trip is inherently lossy. Rather than rewriting the editor, this PR wraps it with a pre/post-process layer that captures the user's source intent, hands a normalised form to Tiptap for editing, and reverses the lossy parts on the way out.
Shared config
buildTiptapExtensions()is the single source of extension config. Production (mountMarkdownEditor) and the test suite both import it, so the regression tests exercise the exact configuration that ships.Pre/post-process layer
preprocessForEditor(input)returns{ editorInput, context }.applyPostProcess(serialized, context)produces the final output. TheRoundTripContextcaptures everything Tiptap loses or mutates:\r\nvs\n)---as Setext heading|in cellsExtensions added
@tiptap/extension-table,-table-row,-table-header,-table-cell— without these, GFM pipe tables collapse to concatenated cell text (the canonical [BUG] Markdown preview auto-save corrupts tables and wikilinks in edit_block target files #437 corruption).strike: falsein StarterKit — was over-escaping~to\~.linkify: falsein tiptap-markdown — was wrapping bare URLs in<…>autolink brackets.Post-process repairs (run in order)
restoreWikiLinks—[[Page]]syntax (existing, unchanged).[\x`](url)andX` patterns. Tiptap drops the URL on these; the placeholder trick bypasses parsing.**…\code`…**` patterns. ProseMirror's flat-mark schema can't represent these cleanly.\|in cells is required for cell integrity; the placeholder protects it from Tiptap's serializer.unescapeSafeChars— line-aligned removal of\[,\],\~escapes Tiptap added that aren't in the source.unescapeHtmlEntitiesInProse— line-aligned removal of<,>,&Tiptap's HTML output emitted.restoreTrailingHardBreaks— re-add two-space hard-break syntax where the source had it.restoreTableSeparatorStyle—|---|vs| --- |, restored to source style.restoreBulletMarkers—*/-/+bullet markers, mapped positionally.restoreSoftBreaks— paragraph-internal\nboundaries that Tiptap collapses to a space (or to nothing, between punctuation and emoji). Recognises list-item lazy continuations as a special case.collapseBlockSeparators— strip blank lines Tiptap inserts between block elements that were adjacent in the source.Test infrastructure
controller.ts:computeEditBlocksis now exported so the edit-diff test exercises the actual autosave logic, including the 70% whole-file-rewrite trigger.roundTripMarkdown(input)is a shared helper that mounts → serializes → applies post-process. Both test suites use it, so changes to the production path can't drift from the test path.Autosave-scoping approach (merged from edgarssskore's branch)
hasUserEditedguard (a66e9f6)In
mountMarkdownEditor, ahasUserEditedflag is flipped on the first real user interaction (beforeinput,paste,cut,drop, or any toolbar action — link, format, heading change).onUpdateno-ops until the flag is set, preventing spurious autosaves on the parse-and-mount cycle.Edit-range scoping (
a23ae76)The editor tracks line ranges that have actually changed between snapshots via
computeSerializedEditRanges(LCS-based, with a hash-anchored fallback for very large files). These ranges are passed throughonChangeto the controller, which forwards them tocomputeEditBlocks. The diff hunks are filtered to only those that intersect a user-edited range. Even if Tiptap's parse drifts a few unrelated lines, only the user's actual edit lands on disk.Also adds
splitOversizedEditBlocksto break very large edit blocks into ≤40-line chunks before sending them toedit_block, so the underlying tool stays responsive on large rewrites.Failure modes covered (with isolating tests)
Every reproducer in the strict suite is a minimal fixture for one observed failure mode. Categorised by source:
From #437 / #440:
testPipeTableSurvivesRoundTrip)~→\~(testTildeIsNotEscaped)testAdjacentHeadingsKeepOriginalSpacing)testWikilinkSurvivesRoundTrip)testTrailingNewlineSurvives)testCombinedBugReportFile)testYamlFrontmatterSurvives)[x]task brackets (testSquareBracketsNotEscaped)testUnderscoresNotEscaped)~/pathpaths (testTildePathNotEscaped)testFrontmatterListItem)testCrlfPreserved)testReadmeStyleFileNotCollapsed)testTableInsideRealisticDoc)From
~/work/best-value-ai/README.md:testBareUrlNotAutoLinked)testEmojiPrefixedSoftBreaksRestored)testLinkInTableCellSurvivesRoundTrip)*bullet marker (testStarBulletMarkerPreserved)From
~/.desktop-commander/skills/*/SKILL.md:testRelativePathLinksSurvive)<in prose entity-escaped (testLessThanInProseNotEscaped)testTrailingHardBreakWhitespacePreserved)testBoldAroundInlineCodePreserved)\|in table cells (testEscapedPipeInTableCellPreserved)testListItemWithContinuationLine)Choice between this and #442
This PR keeps Tiptap and works around its rough edges. PR #442 replaces Tiptap with CodeMirror.
If the team's preference is to keep Tiptap, this is ready to land. If the preference is to own the editor surface end-to-end, #442 is the right call.
Closes #437. Closes #440. Closes #449.