Improvements to apply_diff #52
Merged
Conversation
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
mrubens
force-pushed
the
diff_edit_improvements
branch
from
4370532 to
72c5c71
Compare
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 4370532207d589d6e305b5a5ee160b2bacf56e6a in 26 seconds
More details
- Looked at
107lines of code in4files - Skipped
1files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/core/prompts/system.ts:94
- Draft comment:
Ensure the usage example for 'apply_diff' matches the specified format 'diff -U0'. - Reason this comment was not posted:
Comment did not seem useful.
2. src/core/Cline.ts:1235
- Draft comment:
Use async/await instead of .then() for better readability and maintainability. This is from our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d581809f9af3fdba09412ef6 - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_wVurl2BTZSzlonxS
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Contributor
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 72c5c71 in 36 seconds
More details
- Looked at
107lines of code in4files - Skipped
1files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. src/core/prompts/system.ts:72
- Draft comment:
The description mentions using 'diff -U0', but also states not to include line numbers. 'diff -U0' includes line numbers, which contradicts this instruction. Consider clarifying the expected format. - Reason this comment was not posted:
Comment did not seem useful.
2. src/core/Cline.ts:1235
- Draft comment:
The description mentions using 'diff -U0', but also states not to include line numbers. 'diff -U0' includes line numbers, which contradicts this instruction. Consider clarifying the expected format. - Reason this comment was not posted:
Marked as duplicate.
3. src/core/Cline.ts:1235
- Draft comment:
Consider using async/await instead of .then() for handling asynchronous operations. This makes the code more readable and is part of our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d581809f9af3fdba09412ef6 - Reason this comment was not posted:
Comment was on unchanged code.
4. CHANGELOG.md:5
- Draft comment:
The PR description is missing details on how the changes have been tested and any related tickets/issues. Please include these details as per our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d58180429648e12f7977d646 - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_OOwtAi0eH9M68S2M
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Contributor
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 9cc311d in 11 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. src/utils/sound.ts:23
- Draft comment:
ChangingisSoundEnabledfromtruetofalsemight affect the default behavior of the application. Ensure this change is intentional. - Reason this comment was not posted:
Confidence changes required:50%
The change from true to false for isSoundEnabled might be intentional for testing or default behavior, but it should be noted.
Workflow ID: wflow_Bw8HNDJrqVx7uAZR
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
mrubens
commented
Dec 8, 2024
| } | ||
|
|
||
| let isSoundEnabled = true | ||
| let isSoundEnabled = false |
Collaborator
Author
There was a problem hiding this comment.
Couldn't help sneaking this one in, just to be safe.
Contributor
There was a problem hiding this comment.
feedback from https://discord.com/channels/1275535550845292637/1275535550845292640/1315443495711936634
alexamd13 wrote about sound effects:
overly unnecessary for your average user, if they see a task has been complete, they'll already see it via text
Contributor
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 27b4bb2 in 11 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. CHANGELOG.md:5
- Draft comment:
Add a period at the end of the sentence for consistency with other entries.
- Fix bug where diffs were not being applied correctly and try Aider's [unified diff prompt](https://github.com/Aider-AI/aider/blob/3995accd0ca71cea90ef76d516837f8c2731b9fe/aider/coders/udiff_prompts.py#L75-L105).
- Reason this comment was not posted:
Confidence changes required:10%
The changelog entry for version 2.1.14 is missing a period at the end of the sentence, which is inconsistent with the rest of the entries.
2. CHANGELOG.md:5
- Draft comment:
Ensure consistency in punctuation. Add a period at the end of the sentence for version 2.1.14 entry.
- Fix bug where diffs were not being applied correctly and try Aider's [unified diff prompt](https://github.com/Aider-AI/aider/blob/3995accd0ca71cea90ef76d516837f8c2731b9fe/aider/coders/udiff_prompts.py#L75-L105).
- Reason this comment was not posted:
Confidence changes required:33%
The changelog entry for version 2.1.14 is missing a period at the end of the sentence. This is a minor style issue but should be consistent with other entries.
Workflow ID: wflow_nV3FPyAicS9d7DWK
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Contributor
There was a problem hiding this comment.
❌ Changes requested. Incremental review on a5438028a193294c8669e77008b0e68d97669df4 in 30 seconds
More details
- Looked at
65lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. src/integrations/misc/extract-text.ts:57
- Draft comment:
TheaddLineNumbersfunction is defined here and also insystem.ts. Consider refactoring to avoid code duplication by moving this function to a shared utility module. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests a refactor to avoid code duplication, which aligns with our Development Standards about using shared utilities for common operations. However, the diff does not provide evidence of the function's presence in another file, so the comment is speculative based on the diff alone.
The comment assumes the presence of the function in another file without evidence in the diff. It might be speculative unless verified with the full codebase.
The comment aligns with the principle of reducing redundancy, which is a valid code quality concern. However, without evidence in the diff, it might not be actionable based on the diff alone.
The comment should be removed as it is speculative without evidence in the diff, despite being a valid code quality suggestion.
Workflow ID: wflow_bNHqUjRrEIbTUaZv
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Contributor
There was a problem hiding this comment.
The addLineNumbers function and other new functionalities should have corresponding unit tests to ensure they work as expected. According to our Development Standards, unit tests are mandatory for new functionality with a minimum acceptable test coverage of 80%.
mrubens
force-pushed
the
diff_edit_improvements
branch
from
a543802 to
3df3505
Compare
Contributor
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 3df3505bbfbc641ba26b55e70b036bba773d7d24 in 20 seconds
More details
- Looked at
97lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/core/webview/__tests__/ClineProvider.test.ts:271
- Draft comment:
Consider adding more comprehensive tests for theaddLineNumbersfunction to cover edge cases, such as empty files or files with varying line lengths. - Reason this comment was not posted:
Confidence changes required:50%
The functionaddLineNumbersis used multiple times in the fileextract-text.ts. It is a good practice to ensure that the function is well-tested. However, the current test inClineProvider.test.tsonly checks for a specific case. It would be beneficial to have more comprehensive tests for this function.
2. src/core/webview/__tests__/ClineProvider.test.ts:271
- Draft comment:
Ensure that all new functionalities have at least 80% test coverage. This is in line with our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d58180f7b224d6db54dab6b5 - Reason this comment was not posted:
Confidence changes required:50%
The test file includes a test for checking if file content includes line numbers. This is a good practice to ensure the functionality works as expected. However, it's important to ensure that all new functionalities have at least 80% test coverage, as per our Development Standards.
Workflow ID: wflow_4z3GnLr3Jldu7Uq6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
mrubens
force-pushed
the
diff_edit_improvements
branch
from
3df3505 to
32e0bd8
Compare
Contributor
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 32e0bd838c6931c21752829bd332ce76c99a2521 in 47 seconds
More details
- Looked at
139lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. src/integrations/misc/extract-text.ts:23
- Draft comment:
Consider logging the error in thecatchblock when checking if a file is binary to aid in debugging. - Reason this comment was not posted:
Confidence changes required:33%
The code inextract-text.tsusescatchto handle errors when checking if a file is binary. This is a good practice to prevent the application from crashing if the check fails. However, the error handling could be more explicit by logging the error for debugging purposes.
2. src/integrations/misc/extract-text.ts:57
- Draft comment:
Consider optimizingaddLineNumbersby calculatingmaxLineNumberWidthonce outside themapfunction to improve performance. - Reason this comment was not posted:
Confidence changes required:33%
TheaddLineNumbersfunction is used in multiple places to add line numbers to file content. This is a good practice for consistency and readability. However, the function could be optimized by avoiding recalculating the maximum line number width for each line.
3. src/core/Cline.ts:1134
- Draft comment:
Consider optimizingaddLineNumbersby calculatingmaxLineNumberWidthonce outside themapfunction to improve performance. This applies to other usages in this file as well. - Reason this comment was not posted:
Confidence changes required:33%
TheaddLineNumbersfunction is used in multiple places inCline.tsto add line numbers to file content. This is a good practice for consistency and readability. However, the function could be optimized by avoiding recalculating the maximum line number width for each line.
4. src/core/Cline.ts:1246
- Draft comment:
Consider optimizingaddLineNumbersby calculatingmaxLineNumberWidthonce outside themapfunction to improve performance. This applies to other usages in this file as well. - Reason this comment was not posted:
Confidence changes required:33%
TheaddLineNumbersfunction is used in multiple places inCline.tsto add line numbers to file content. This is a good practice for consistency and readability. However, the function could be optimized by avoiding recalculating the maximum line number width for each line.
5. src/integrations/misc/extract-text.ts:23
- Draft comment:
Use async/await instead of .then() for promise handling to improve readability and maintainability. This is in accordance with our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d581809f9af3fdba09412ef6 - Reason this comment was not posted:
Comment was on unchanged code.
6. src/integrations/misc/extract-text.ts:24
- Draft comment:
Use async/await instead of .then() for promise handling to improve readability and maintainability. This is in accordance with our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d581809f9af3fdba09412ef6 - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_aWWxHeHgN9VdQGEF
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
mrubens
force-pushed
the
diff_edit_improvements
branch
from
32e0bd8 to
538fedb
Compare
Contributor
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 538fedb in 35 seconds
More details
- Looked at
139lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. src/integrations/misc/extract-text.ts:25
- Draft comment:
Consider callingaddLineNumbersonce after extracting the content instead of within each file type handler to improve performance. - Reason this comment was not posted:
Confidence changes required:50%
The addition of line numbers to the file content is a new feature. However, the functionaddLineNumbersis being called multiple times in theextractTextFromFilefunction and its related functions. This could lead to performance issues if the content is large, as the function is called for every file type that is not binary. It would be more efficient to calladdLineNumbersonce after the content is fully extracted.
2. src/integrations/misc/extract-text.ts:35
- Draft comment:
Consider callingaddLineNumbersonce after extracting the content instead of within each file type handler to improve performance. This comment applies to other similar usages in this file. - Reason this comment was not posted:
Confidence changes required:50%
TheaddLineNumbersfunction is being used in multiple places within theextractTextFromFilefunction and its related functions. This could lead to performance issues if the content is large, as the function is called for every file type that is not binary. It would be more efficient to calladdLineNumbersonce after the content is fully extracted.
3. src/integrations/misc/extract-text.ts:40
- Draft comment:
Consider callingaddLineNumbersonce after extracting the content instead of within each file type handler to improve performance. This comment applies to other similar usages in this file. - Reason this comment was not posted:
Confidence changes required:50%
TheaddLineNumbersfunction is being used in multiple places within theextractTextFromFilefunction and its related functions. This could lead to performance issues if the content is large, as the function is called for every file type that is not binary. It would be more efficient to calladdLineNumbersonce after the content is fully extracted.
4. src/integrations/misc/extract-text.ts:54
- Draft comment:
Consider callingaddLineNumbersonce after extracting the content instead of within each file type handler to improve performance. This comment applies to other similar usages in this file. - Reason this comment was not posted:
Confidence changes required:50%
TheaddLineNumbersfunction is being used in multiple places within theextractTextFromFilefunction and its related functions. This could lead to performance issues if the content is large, as the function is called for every file type that is not binary. It would be more efficient to calladdLineNumbersonce after the content is fully extracted.
5. src/core/Cline.ts:12
- Draft comment:
The PR description is missing comprehensive details. Please include: - What is being changed?
- Why is it being changed?
- How has it been tested?
- Any related tickets/issues?
- Are there visual/UI changes? Provide before & after screenshots.
Refer to our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d58180429648e12f7977d646 - Reason this comment was not posted:
Comment did not seem useful.
6. src/core/webview/__tests__/ClineProvider.test.ts:76
- Draft comment:
Ensure that data-testid attributes are used for all UI elements under test, and use camelCase for ID values. Refer to our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d58180f7b224d6db54dab6b5 - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_yUKcIrklAgR39YUg
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Contributor
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 955660c in 18 seconds
More details
- Looked at
82lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/core/prompts/system.ts:99
- Draft comment:
The example diff header should have three '+' signs for the modified file path, not four. This is a minor formatting issue but could cause confusion.
+++ src/utils/helper.ts
- Reason this comment was not posted:
Confidence changes required:50%
The updated diff format instructions are clear and detailed, but there's a minor inconsistency in the example provided.
2. src/core/prompts/system.ts:71
- Draft comment:
Ensure proper error handling in asynchronous functions. Consider using try/catch blocks to handle potential errors in the 'addCustomInstructions' function. This is from our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d58180bc982fd2d560354e47 - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_C2gUXGxvbMUmG6eC
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Contributor
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d84b9724257ecbce0f25693963e34554918fb1b8 in 44 seconds
More details
- Looked at
113lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. src/core/Cline.ts:49
- Draft comment:
The functionshowOmissionWarningis removed but still used in the code. This will cause a runtime error. Consider re-adding the import or removing the usage of the function. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. src/core/Cline.ts:1119
- Draft comment:
Consider using async/await instead of .then() for promise handling to improve readability and maintainability. This is in line with our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d581809f9af3fdba09412ef6 - Reason this comment was not posted:
Comment looked like it was already resolved.
3. src/core/Cline.ts:1127
- Draft comment:
Consider using async/await instead of .then() for promise handling to improve readability and maintainability. This is in line with our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d581809f9af3fdba09412ef6 - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_rCwsKnd6Tcr637hN
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
stea9499
approved these changes
Dec 8, 2024
mrubens
force-pushed
the
diff_edit_improvements
branch
from
d84b972 to
4846796
Compare
Contributor
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 4846796 in 57 seconds
More details
- Looked at
127lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. src/core/Cline.ts:49
- Draft comment:
The functionshowOmissionWarningis used but not imported. Consider importing it or removing its usage if it's no longer needed. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. src/integrations/editor/detect-omission.ts:7
- Draft comment:
ThedetectCodeOmissionfunction may not detect all omissions if keywords are not present in comments. Consider enhancing the logic to handle more cases. - Reason this comment was not posted:
Confidence changes required:50%
ThedetectCodeOmissionfunction is used in multiple places inCline.tsto check for code omissions. However, the logic indetectCodeOmissionmight not be robust enough to detect all types of omissions, especially if the omission keywords are not present in comments. This could lead to false negatives.
3. src/core/Cline.ts:1101
- Draft comment:
Consider logging an error message whenapplyPatchreturns false for better debugging. - Reason this comment was not posted:
Confidence changes required:30%
Theapply_difffunction inCline.tsusesapplyPatchfrom thedifflibrary. IfapplyPatchreturns false, it indicates the patch could not be applied. It would be helpful to log this event for debugging purposes.
4. webview-ui/src/components/settings/SettingsView.tsx:304
- Draft comment:
The description fordiffEnabledcorrectly mentions the rejection of truncated full-file edits, aligning with the PR changes. - Reason this comment was not posted:
Confidence changes required:0%
TheSettingsView.tsxfile contains a description for thediffEnabledcheckbox. The description mentions that truncated full-file edits will be automatically rejected, which aligns with the changes made in the PR.
5. src/core/Cline.ts:1120
- Draft comment:
Consider using async/await instead of .then() for promise handling to improve readability and maintainability. This is in line with our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d581809f9af3fdba09412ef6 - Reason this comment was not posted:
Marked as duplicate.
6. src/core/Cline.ts:1124
- Draft comment:
Consider using async/await instead of .then() for promise handling to improve readability and maintainability. This is in line with our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d581809f9af3fdba09412ef6 - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_ZXCmzknIUjExm1XO
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| "Potential code truncation detected. This happens when the AI reaches its max output limit.", | ||
| "Follow this guide to fix the issue", | ||
| ) | ||
| .then((selection) => { |
Contributor
There was a problem hiding this comment.
Consider using async/await instead of .then() for promise handling to improve readability and maintainability. This is in line with our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d581809f9af3fdba09412ef6
roomote
pushed a commit
that referenced
this pull request
Jul 8, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Lots of stuff here that we should probably read through... https://github.com/Aider-AI/aider/blob/42ae279b91544ebcaafd4cc595d35ee2a9f460eb/aider/website/docs/unified-diffs.md
Important
Fix file saving bug and enhance diff application process in
Cline.ts, with updated instructions insystem.ts.Cline.tsafter applying diffs.apply_diffinCline.tswith a more robust process, including user feedback and content verification.apply_diffinstructions insystem.tsfor correct diff formatting.extract-text.tsto aid in diff creation.2.1.14inpackage.jsonandCHANGELOG.md.isSoundEnabledtofalseinsound.ts.SettingsView.tsxto reflect changes in diff and sound settings.This description was created by
for 4846796. It will automatically update as commits are pushed.