fix(light-touch): preserve original blank line counts during merge

[peterjthomson]

Problem

The Light Touch mode's mergeWithOriginal function was causing unwanted diffs for files with intentional double (or multiple) blank lines.

When editing a file like:

# Heading


Paragraph (note: 2 blank lines above)

Any edit elsewhere in the file would collapse the double blank lines to a single blank line on save.

Root Cause

The merge algorithm:

1. Matches non-blank lines via LCS (# Heading, Paragraph)
2. Takes blank lines from regenerated markdown (Muya typically produces 1 blank line)
3. Discards the original's 2 blank lines

Solution

Updated mergeWithOriginal to:

• Preserve original gap lines: When there are unmatched lines between matched content in the original (typically blank lines), preserve them exactly instead of substituting regenerated blank lines
• Insert new content correctly: When new non-blank content is added between existing paragraphs, preserve the original blank line count before inserting the new content
• Handle tail similarly: Same logic applies to content after the last matched line

Testing

• All 32 existing unit tests pass
• Manual testing confirms double blank lines are preserved after edit+save cycles

Documentation

Updated docs/todo/save-light-touch.md to mark this issue as fixed.

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

[peterjthomson]

@claude review

[claude]

Claude finished @peterjthomson's task in 1m 28s —— View job

---

Code Review

Todo List

• Read repository guidelines (CLAUDE.md)
• Review changed files and understand the implementation
• Analyze code for bugs, edge cases, and correctness
• Check code quality and maintainability
• Provide comprehensive review feedback

---

Review Summary

This PR successfully addresses the double blank line preservation issue in Light Touch mode. The implementation is well-designed and the fix is targeted and effective. I found no blocking issues, but have some observations and suggestions for consideration.

What Changed

The mergeWithOriginal function (src/renderer/src/store/editor.js:1593-1674) was updated to:

• Track both original and regenerated gap lines between matched content
• Preserve original blank line counts when no new content is added
• Insert new content while maintaining original blank line spacing

Strengths

1. Targeted fix: The solution directly addresses the root cause identified in the PR description
2. Preserves intent: Distinguishes between "original had double blank lines" vs "new content being added"
3. Clear logic: The gap handling logic is well-structured with clear comments
4. Documentation updated: Properly marks the issue as fixed in save-light-touch.md

Observations & Edge Cases to Consider

1. Mixed blank lines and new content (src/renderer/src/store/editor.js:1610-1625)

When there's both original blank lines AND new content in a gap, the current logic preserves origBlankCount blank lines before inserting new content:

if (newContent.length > 0) {
  // Preserve original blank lines before the new content
  for (let i = 0; i < origBlankCount; i++) {
    resultLines.push('')
  }
  resultLines.push(...newContent)
}

Question: Should blank lines come before or after new content? Consider this scenario:

Original:
# Heading


Paragraph

New content adds a bullet between them:
# Heading
- New bullet
Paragraph

Current behavior would produce:

# Heading


- New bullet
Paragraph

Is this the desired behavior, or should the new content determine its own spacing? This may be working as intended, but worth verifying with manual testing.

2. Performance consideration (src/renderer/src/store/editor.js:1612, 1648)

The code uses .filter() multiple times on the same arrays:

const origBlankCount = origGapLines.filter((l) => normalizeLine(l) === '').length
const newContent = regenGapLines.filter((l) => normalizeLine(l) !== '')

For most markdown files this is negligible, but it does iterate the arrays twice. A minor optimization would be to count/filter in a single pass, but this is not critical.

3. Tail handling symmetry (src/renderer/src/store/editor.js:1641-1667)

The tail handling logic mirrors the gap handling, which is good for consistency. However, the comment on line 1666 mentions "rely on original trailing newlines preservation below" - this is handled at line 1670-1672, so the logic is correct.

4. Edge case: All blank lines (src/renderer/src/store/editor.js:1623-1624)

When origGapLines contains only blank lines and there's no new content, they're preserved:

} else {
  // No new content - preserve original gap lines exactly (keeps double blanks)
  resultLines.push(...origGapLines)
}

This is correct behavior for the stated goal. ✓

5. Normalized vs raw blank lines

The code uses normalizeLine(l) === '' to detect blank lines, which strips trailing whitespace. This means a line with only spaces/tabs is treated as blank. This seems intentional and correct for markdown semantics.

Code Quality

• Readability: ✓ Comments are clear and explain the logic
• Maintainability: ✓ The dual handling of gaps and tail reduces code duplication
• Naming: ✓ Variable names like origGapLines, regenGapLines, newContent are descriptive

Testing

The PR description mentions "All 32 existing unit tests pass" and "Manual testing confirms double blank lines are preserved."

Recommendation: Consider adding explicit unit tests for:

• Double blank lines preserved when editing elsewhere ✓ (covered by manual testing)
• Triple or more blank lines
• Blank lines with new content insertion (the mixed case discussed above)
• Edge case: file with only blank lines
• Edge case: new content after multiple consecutive blank lines

Since the PR mentions tests pass, the changes are likely backwards compatible. However, explicit tests for the new behavior would help prevent regressions.

Documentation

The update to docs/todo/save-light-touch.md correctly marks the issue as fixed with a strikethrough and explanation. Well done.

Security & Performance

• Security: No security concerns identified
• Performance: The LCS algorithm is O(n*m) which is acceptable for typical markdown files. The additional filtering operations don't significantly impact performance.

Verdict

✅ Approve - This is a well-implemented fix that addresses the stated problem without introducing obvious bugs. The code is clear, well-commented, and maintains consistency with the existing Light Touch architecture.

The observations above are minor points for consideration rather than blocking issues. The "mixed content" scenario (observation #1) might be worth verifying with manual testing to ensure it behaves as intended, but the current logic seems reasonable.

---