rejected edits trailing whitespaces #4687

panyamkeerthana · 2025-03-17T02:16:47Z

Description

Earlier trailing whitespace was stripped in streamDiffLines.ts before generating diffs, causing mismatches when rejecting a diff in the editor.
This fix updates linesMatchPerfectly to compare lines using trimEnd(), ensuring that trailing whitespace differences do not interfere with accurate diff rejection.

Screenshots

Granite-Code/granite-code#50

netlify · 2025-03-17T02:17:11Z

✅ Deploy Preview for continuedev canceled.

Name	Link
🔨 Latest commit	`3ba5452`
🔍 Latest deploy log	https://app.netlify.com/sites/continuedev/deploys/67dc74500b5731000700b87c

extensions/vscode/src/quickEdit/EditDecorationManager.ts

owtaylor · 2025-03-17T16:08:51Z

core/diff/util.ts

+//checks if two lines match exactly ignoring trailing whitespace differences
+//this prevents false mismatches in diff handling when trailing spaces are stripped
+
+  return lineA.trimEnd() === lineB.trimEnd() && lineA !== "";


This fixes up one case where we are comparing oldLines to newLines, but checking over the code, this isn't the only place where we might have issues:

in matchLine(), after calling linesMatchPerfectly(), we make more references to oldLines[i] that potentially might need changes.

In streamDiff(), we have:

if (oldLinesCopy[0] !== newLine) { yield { type: "new", line: newLine }; } else { isLineRemoval = true; }

which looks suspicious, but I'm pretty sure can never actually be hit: if oldLinesCopy[0] === newLine then isPerfectMatch should have been true and the case will be "same" not "old". Needs double checking.

But the takeaway for me here is that the test cases need updating to test different cases where lines differ only in trailing whitespace. From the continue/core directory, you can run:

$ npm test diff/streamDiff.test.ts

to run streamDiff test cases. Add a test case to check the linesMatchPerfectly case, add a test case case that exercises: // This is a way to fix indentation, but only for sufficiently long lines to avoid matching whitespace or short lines when the indentation changes and there is a change in whitespace.

Thanks for the review, I have made the following changes:

-Added a test case for oldLinesCopy[0] === newLine that confirmed it’s already handled as "same" or "old", so the else statement was unnecessary and removed.
-Trimmed oldLines throughout and added a test to ensure trailing whitespaces are correctly treated as "same", preventing incorrect diffs.
-Added a test to verify that indentation changes in long lines don’t cause unnecessary diffs.

owtaylor · 2025-03-18T20:03:49Z

core/diff/streamDiff.test.ts

+  });
+
+  test("ignores indentation changes for sufficiently long lines", async () => {
+    const oldLines = ["   first item", " second arg", "third param", " fourth val "];


I would try to test this more comprehensively - to test that indentation changes on short lines are considered changes, until one line with length > 8 is encountered, then they are ignored, even for short lines.

Importantly, I'd also check here that the same behavior is seen when the old and new lines differ in trailing whitespace - that's the point of adding the check as part of this patch, after all.

Something like:

test.each([false, true])( `ignores identation changes for sufficiently long lines", async (trailingWhitespace) => { [...] if (trailingWhitespace) oldLines = oldLines.map(line => line + " "); [...] });

I'm sure there's a more stylish way to do that.

owtaylor · 2025-03-18T20:11:09Z

core/diff/util.ts

    // Don't match end bracket lines if too far away
    if (i > 4 && isEndBracket) {
      return { matchIndex: -1, isPerfectMatch: false, newLine };
    }

-    if (linesMatchPerfectly(newLine, oldLines[i])) {
+    if (linesMatchPerfectly(newLine, oldLineTrimmed)) {
      return { matchIndex: i, isPerfectMatch: true, newLine };


I think this probably should be oldLines[i] not newLine? The goal here should be that when we return same - we actually have the old line, not a version that differs in trailing whitespace. First try to make a test case that fails, then change this.

The test cases I ran for this scenario passed. In streamDiff():

case "same": yield { type, line: oldLinesCopy.shift()! }; break;

So, regardless of what matchLine() returns for newLine, the line that is actually output is the one removed from oldLinesCopy (which is the original old line with the trailing whitespaces). But I can change it to oldLine[i] to maintain logic and uniformity

Turned out that this very much needed to be newLine - the calling code relies on this only being different from the passed in newline when the leading indentation changed. Sorry for the confusion!

All this testing has only made the whitespace and indentation handling more robust!

owtaylor · 2025-03-18T20:11:39Z

core/diff/util.ts

        (permissiveAboutIndentation || newLine.trim().length > 8)
      ) {
        return {
          matchIndex: i,
          isPerfectMatch: true,
-          newLine: oldLines[i],
+          newLine: oldLineTrimmed,


Similarly, I think this should be oldLines[i]?

I'll change that to oldLine[i] here as well

owtaylor · 2025-03-18T20:14:27Z

core/diff/streamDiff.ts

@@ -63,8 +63,6 @@ export async function* streamDiff(

        if (oldLinesCopy[0] !== newLine) {
          yield { type: "new", line: newLine };
-        } else {
-          isLineRemoval = true;
        }


You should remove the if/else entirely and just leave the yield, then since isLineRemoval can never be set, it needs to be removed as well.

owtaylor · 2025-03-18T20:23:09Z

core/diff/streamDiff.test.ts

@@ -223,6 +223,51 @@ describe("streamDiff(", () => {
    await expectDiff("fastapi-tabs-vs-spaces.py");
  });

+
+  test(" condition where oldLinesCopy[0]=== newLine", async () => {


I think this was useful during development of your patch, but since this code ends up removed in your patches, I'd suggest removing the test case as well.

owtaylor

Just a few more comments

owtaylor · 2025-03-19T19:37:16Z

core/edit/streamDiffLines.ts

@@ -79,7 +79,7 @@ export async function* streamDiffLines(
  }

  // Trim end of oldLines, otherwise we have trailing \r on every line for CRLF files
-  oldLines = oldLines.map((line) => line.trimEnd());
+  oldLines = oldLines.map((line) => line);


This can just go away, along with the comment

core/diff/streamDiff.ts

owtaylor · 2025-03-19T19:42:52Z

core/diff/streamDiff.test.ts

+        "short",
+        "middle",
+        "a long enough line",
+        "indented short",


"indented short".length is going to be > 8 - this needs to be "short2" or something to actually test that handlng.

owtaylor · 2025-03-19T19:48:47Z

core/diff/util.ts

      // This is a way to fix indentation, but only for sufficiently long lines to avoid matching whitespace or short lines
      if (
-        newLine.trimStart() === oldLines[i].trimStart() &&
+        newLine.trimStart() === oldLineTrimmed.trimStart() &&


I don't think we can assume that newLine is trimmed at the end, so this should be newLine.trim()? [make sure the test case tests this - one line that is supposed to not reindented should have a trailing space on the new]

Instead, as discussed I created newLineTrimmed.

newLineTrimmed.trimStart() === oldLineTrimmed.trimStart()

trailing whitespace was stripped before generating diffs, causing mismatches when rejecting a diff in the editor. this fix ensures that trailing whitespace differences do not interfere with accurate diff rejection.

since `oldLinesCopy[0] === newLine` should always be classified as `"same"` or `"old"` in the earlier block, the `else` statement (which sets `isLineRemoval = true`)feels redundant and never actually executed.

owtaylor · 2025-03-20T20:44:56Z

Looks good to me now!

tomasz-stefaniak

Thank you for contributing @panyamkeerthana, and @owtaylor for the thorough review!

panyamkeerthana requested a review from a team as a code owner March 17, 2025 02:16

panyamkeerthana requested review from tomasz-stefaniak and removed request for a team March 17, 2025 02:16

panyamkeerthana changed the title ~~Panyamkeerthana/rejected trailing whitespaces~~ rejected edits trailing whitespaces Mar 17, 2025

owtaylor reviewed Mar 17, 2025

View reviewed changes

panyamkeerthana force-pushed the panyamkeerthana/rejected-trailing-whitespaces branch 3 times, most recently from f1a5f04 to 58d8f7c Compare March 18, 2025 19:42

owtaylor reviewed Mar 18, 2025

View reviewed changes

panyamkeerthana force-pushed the panyamkeerthana/rejected-trailing-whitespaces branch 2 times, most recently from fbdda80 to a8953c9 Compare March 19, 2025 15:55

owtaylor reviewed Mar 19, 2025

View reviewed changes

panyamkeerthana force-pushed the panyamkeerthana/rejected-trailing-whitespaces branch 5 times, most recently from f0be628 to 8ebcc0c Compare March 20, 2025 19:51

rejected edit trims the trailing whitespaces

f6fa09b

trailing whitespace was stripped before generating diffs, causing mismatches when rejecting a diff in the editor. this fix ensures that trailing whitespace differences do not interfere with accurate diff rejection.

panyamkeerthana force-pushed the panyamkeerthana/rejected-trailing-whitespaces branch from 8ebcc0c to 4822926 Compare March 20, 2025 19:58

remove redundant code

3ba5452

since `oldLinesCopy[0] === newLine` should always be classified as `"same"` or `"old"` in the earlier block, the `else` statement (which sets `isLineRemoval = true`)feels redundant and never actually executed.

panyamkeerthana force-pushed the panyamkeerthana/rejected-trailing-whitespaces branch from 4822926 to 3ba5452 Compare March 20, 2025 20:02

tomasz-stefaniak approved these changes Mar 21, 2025

View reviewed changes

tomasz-stefaniak merged commit 7d95521 into continuedev:main Mar 21, 2025
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rejected edits trailing whitespaces #4687

rejected edits trailing whitespaces #4687

panyamkeerthana commented Mar 17, 2025

netlify bot commented Mar 17, 2025 •

edited

Loading

owtaylor Mar 17, 2025

panyamkeerthana Mar 18, 2025

owtaylor Mar 18, 2025

owtaylor Mar 18, 2025

panyamkeerthana Mar 19, 2025 •

edited

Loading

owtaylor Mar 20, 2025

panyamkeerthana Mar 20, 2025 •

edited

Loading

owtaylor Mar 18, 2025

panyamkeerthana Mar 19, 2025

owtaylor Mar 18, 2025

owtaylor Mar 18, 2025

owtaylor left a comment

owtaylor Mar 19, 2025

owtaylor Mar 19, 2025

owtaylor Mar 19, 2025

panyamkeerthana Mar 20, 2025

owtaylor commented Mar 20, 2025

tomasz-stefaniak left a comment

rejected edits trailing whitespaces #4687

rejected edits trailing whitespaces #4687

Conversation

panyamkeerthana commented Mar 17, 2025

Description

Screenshots

netlify bot commented Mar 17, 2025 • edited Loading

✅ Deploy Preview for continuedev canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panyamkeerthana Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panyamkeerthana Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owtaylor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owtaylor commented Mar 20, 2025

tomasz-stefaniak left a comment

Choose a reason for hiding this comment

netlify bot commented Mar 17, 2025 •

edited

Loading

panyamkeerthana Mar 19, 2025 •

edited

Loading

panyamkeerthana Mar 20, 2025 •

edited

Loading