Fix #3653: insertContentTool to handle appending content when origina…

[Ruakij]

Related GitHub Issue

Closes: #3653

Description

This PR addresses a bug where sequential insert_content operations would add an extra blank line between the inserted contents, specifically when appending to a file that already ends with a newline.

The action by Roo should be correct, but some editors add a trailing newline. And while the insert-operation after the trailing newline is technically correct, this causes unexpected insertions like in the issue.

The fix introduces special handling within insertContentTool.ts to detect if an append operation is being performed on a file that already has a trailing newline. In such cases, the existing trailing empty string (resulting from split('\n')) is removed before inserting.
This prevents the join('\n') operation from introducing an additional blank line.

Test Procedure

Unit tests have been added to cover this bug fix and ensure proper handling of various newline scenarios.

To verify the fix:

1. Run tests
2. Test the scenario described in the issue
3. Test other insertions which might be affected

Type of Change

• 🐛 Bug Fix: Non-breaking change that fixes an issue.
• ✨ New Feature: Non-breaking change that adds functionality.
• 💥 Breaking Change: Fix or feature that would cause existing functionality to not work as expected.
• ♻️ Refactor: Code change that neither fixes a bug nor adds a feature.
• 💅 Style: Changes that do not affect the meaning of the code (white-space, formatting, etc.).
• 📚 Documentation: Updates to documentation files.
• ⚙️ Build/CI: Changes to the build process or CI configuration.
• 🧹 Chore: Other changes that don't modify src or test files.

Pre-Submission Checklist

• Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
• Scope: My changes are focused on the linked issue (one major feature/fix per PR).
• Self-Review: I have performed a thorough self-review of my code.
• Code Quality:
  • My code adheres to the project's style guidelines.
  • There are no new linting errors or warnings (npm run lint).
  • All debug code (e.g., console.log) has been removed.
• Testing:
  • New and/or updated tests have been added to cover my changes.
  • All tests pass locally (npm test).
  • The application builds successfully with my changes.
• Branch Hygiene: My branch is up-to-date (rebased) with the main branch.
• Changeset: A changeset has been created using npm run changeset if this PR includes user-facing changes or dependency updates.
• Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Screenshots / Videos

Documentation Updates

• No documentation updates are required.

Additional Notes

Get in Touch

Discord: ruakij

---

Important

Fixes bug in insertContentTool.ts to prevent extra blank lines when appending to files ending with a newline, with tests added in insertContentTool.test.ts.

• Behavior:
  • Fixes bug in insertContentTool.ts where appending content to a file ending with a newline added an extra blank line.
  • Removes trailing empty string from lines array if appending to a file with a trailing newline.
• Tests:
  • Adds unit tests in insertContentTool.test.ts to verify appending behavior with and without trailing newlines.
  • Tests cover inserting at beginning, middle, and end of files, and handle various newline scenarios.
• Misc:
  • No changes to documentation or external interfaces.

^{This description was created by for 5bc4d82e9b976915ae3b6af1a3fac02705ec41de. You can customize this summary. It will automatically update as commits are pushed.}

[daniel-lxs]

Hey @Ruakij, thank you for working on this fix!

I left a couple of questions regarding the behavior of the fix on files that intentionally include an empty line at the end.

Let me know what you think!

[daniel-lxs]

Will this fix affect files that intentionally end with a new line?

Usually typescript files always have a trailing \n at the end, while a small detail, it is important to keep to prevent unintended changes to the files.

[Ruakij]

I have tested a few things just now and found some inconsistencies:

insertGroups

• lways adds a trailing \n when original is empty-str (an empty new file)
• wont add a trailing \n when original has \n

cline.diffViewProvider.update:

• Always adds a trailing \n in the initial diff-view, double newlines later get removed
• Has preservation of trailing newline if the original file has one
• Triggers 2-3 diffs? This function is a bit weird to me.

createPrettyPatch or rather diff.createPatch:

• always adds a trailing \n if oldStr didnt have one
  • But its only used for chat-box

This leads to insertContentTool itself not adding the trailing \n because it wasnt technically part of the content, but those get introduced by either insertGroups or ultimately cline.diffViewProvider.update.

Its also currently not possible to add leading or trailing \n in the tool-call as these dont survive the tool-call parsing.

So i am not sure how to proceed with this. The tests correctly cover the insertContentTool, but the mocks are not accurate i suppose and its not actually possible to check for trailing newline or what actually got inserted.

For separation-of-concerns, the insertContentTool shouldnt concern itself with that problematic. The mockCline is a simplified stand-in, so to me thats also perfectly fine.
Not 100% sure what to do with insertGroups though.

[daniel-lxs]

It might be a good idea to test if an intentional empty line is preserved.

Consider adding test cases for:

// File with intentional empty line at end
const originalContent = "Line 1\nLine 2\n\n"; // Should preserve the empty line

// File with multiple intentional empty lines
const originalContent = "Line 1\nLine 2\n\n\n"; // Should preserve both empty lines

These cases would help catch the issue where the current fix incorrectly removes legitimate empty lines.

[daniel-lxs]

Hey @Ruakij I think I worked on this and solved it, can you take a look to the latest version to see if the issue is still happening?

[daniel-lxs]

This seems to be solved by #3550