Repository navigation
feat(lab): createDiffFile builds a DiffViewer file from two versions of a text #7565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+539
−6
Merged
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
ee53b79
feat(lab): parseUnifiedDiff reads a patch into the DiffViewer file model
latxh 939219d
feat(lab): DiffViewer — unified/split file diff for code review
latxh 06b36a6
feat(lab): createDiffFile builds a DiffViewer file from two versions …
latxh c12a36a
Merge main into feat/lab-diff-parse
cixzhang 9186983
Merge feat/lab-diff-parse into feat/lab-diff-viewer
cixzhang 1de5c35
Merge feat/lab-diff-viewer into feat/lab-diff-create-file
cixzhang 6de4c2d
fix(lab): parseUnifiedDiff marks a file renamed only from git rename …
cixzhang 844c4df
Merge feat/lab-diff-parse into feat/lab-diff-viewer
cixzhang ece4b0e
fix(lab): DiffViewer draws no annotation row when renderLineAnnotatio…
cixzhang 213b77c
Merge feat/lab-diff-viewer into feat/lab-diff-create-file
cixzhang b813199
fix(lab): createDiffFile appends its edit script in a loop, and NaN c…
cixzhang 2b96e01
fix(lab): parseUnifiedDiff reads a headerless diff -r binary line as …
cixzhang 8cae7e3
Merge feat/lab-diff-parse into feat/lab-diff-viewer
cixzhang f959076
Merge feat/lab-diff-viewer into feat/lab-diff-create-file
cixzhang c6e8350
fix(lab): parseUnifiedDiff checks a line's shape before the binary-li…
cixzhang 2e675ec
Merge feat/lab-diff-parse into feat/lab-diff-viewer
cixzhang a3e9f5a
Merge feat/lab-diff-viewer into feat/lab-diff-create-file
cixzhang 111b8fc
fix(lab): parseUnifiedDiff reads binary lines with plain string matching
cixzhang 3f5135b
Merge feat/lab-diff-parse into feat/lab-diff-viewer
cixzhang 1ba5e90
Merge feat/lab-diff-viewer into feat/lab-diff-create-file
cixzhang b5a4121
Merge main into feat/lab-diff-viewer
cixzhang 5fb1e64
Merge feat/lab-diff-viewer into feat/lab-diff-create-file
cixzhang b558d0a
fix(lab): DiffViewer's hunk area reads left-to-right on right-to-left…
cixzhang 7e28a06
Merge feat/lab-diff-viewer into feat/lab-diff-create-file
cixzhang d37b688
Merge main into feat/lab-diff-create-file
cixzhang File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,180 @@ | ||
| // Copyright (c) Meta Platforms, Inc. and affiliates. | ||
|
|
||
| import {describe, it, expect} from 'vitest'; | ||
| import {createDiffFile} from './createDiffFile'; | ||
| import type {DiffViewerFile} from './diffTypes'; | ||
|
|
||
| /** Rebuild both sides from the hunks of a full-context diff. */ | ||
| function sides(file: DiffViewerFile) { | ||
| const oldSide: string[] = []; | ||
| const newSide: string[] = []; | ||
| for (const hunk of file.hunks) { | ||
| for (const line of hunk.lines) { | ||
| if (line.kind !== 'added') { | ||
| oldSide.push(line.content); | ||
| } | ||
| if (line.kind !== 'removed') { | ||
| newSide.push(line.content); | ||
| } | ||
| } | ||
| } | ||
| return {oldSide, newSide}; | ||
| } | ||
|
|
||
| describe('createDiffFile', () => { | ||
| it('returns no hunks for identical text', () => { | ||
| const file = createDiffFile({oldValue: 'a\nb\n', newValue: 'a\nb\n'}); | ||
| expect(file).toMatchObject({status: 'modified', hunks: []}); | ||
| }); | ||
|
|
||
| it('finds a minimal change with numbered lines and a header', () => { | ||
| const file = createDiffFile({ | ||
| oldValue: 'a\nb\nc\n', | ||
| newValue: 'a\nB\nc\n', | ||
| path: 'x.txt', | ||
| }); | ||
| expect(file.oldPath).toBe('x.txt'); | ||
| expect(file.newPath).toBe('x.txt'); | ||
| expect(file.hunks).toHaveLength(1); | ||
| const [hunk] = file.hunks; | ||
| expect(hunk.header).toBe('@@ -1,3 +1,3 @@'); | ||
| expect( | ||
| hunk.lines.map(l => [ | ||
| l.kind, | ||
| l.content, | ||
| l.oldLineNumber, | ||
| l.newLineNumber, | ||
| ]), | ||
| ).toEqual([ | ||
| ['context', 'a', 1, 1], | ||
| ['removed', 'b', 2, null], | ||
| ['added', 'B', null, 2], | ||
| ['context', 'c', 3, 3], | ||
| ]); | ||
| }); | ||
|
|
||
| it('round-trips both texts with enough context', () => { | ||
| const oldValue = 'one\ntwo\nthree\nfour\nfive\nsix\n'; | ||
| const newValue = 'zero\none\nthree\nfour\n4.5\nfive\nsix\nseven\n'; | ||
| const file = createDiffFile({oldValue, newValue, contextLines: 100}); | ||
| const {oldSide, newSide} = sides(file); | ||
| expect(oldSide).toEqual(oldValue.trimEnd().split('\n')); | ||
| expect(newSide).toEqual(newValue.trimEnd().split('\n')); | ||
| }); | ||
|
|
||
| it('splits distant changes into separate hunks', () => { | ||
| const base = Array.from({length: 30}, (_, i) => `line ${i + 1}`); | ||
| const next = [...base]; | ||
| next[1] = 'changed 2'; | ||
| next[25] = 'changed 26'; | ||
| const file = createDiffFile({ | ||
| oldValue: base.join('\n'), | ||
| newValue: next.join('\n'), | ||
| contextLines: 2, | ||
| }); | ||
| expect(file.hunks.map(h => h.header)).toEqual([ | ||
| '@@ -1,4 +1,4 @@', | ||
| '@@ -24,5 +24,5 @@', | ||
| ]); | ||
| }); | ||
|
|
||
| it('marks added and deleted files from a null path', () => { | ||
| const added = createDiffFile({ | ||
| oldValue: '', | ||
| newValue: 'a\nb\n', | ||
| oldPath: null, | ||
| newPath: 'new.txt', | ||
| }); | ||
| expect(added).toMatchObject({status: 'added', oldPath: null}); | ||
| expect(added.hunks[0].header).toBe('@@ -0,0 +1,2 @@'); | ||
|
|
||
| const deleted = createDiffFile({ | ||
| oldValue: 'a\n', | ||
| newValue: '', | ||
| oldPath: 'old.txt', | ||
| newPath: null, | ||
| }); | ||
| expect(deleted).toMatchObject({status: 'deleted', newPath: null}); | ||
| expect(deleted.hunks[0].header).toBe('@@ -1 +0,0 @@'); | ||
| }); | ||
|
|
||
| it('keeps a minimal diff when changes sit between long shared runs', () => { | ||
| const base = Array.from({length: 3000}, (_, i) => `line ${i + 1}`); | ||
| const next = [...base]; | ||
| next[999] = 'changed 1000'; | ||
| next.splice(2000, 1); | ||
| const file = createDiffFile({ | ||
| oldValue: base.join('\n'), | ||
| newValue: next.join('\n'), | ||
| contextLines: 0, | ||
| }); | ||
| expect(file.hunks.map(h => h.header)).toEqual([ | ||
| '@@ -1000 +1000 @@', | ||
| '@@ -2001 +2000,0 @@', | ||
| ]); | ||
| }); | ||
|
|
||
| it('shows a rewrite too large to search as one replaced block', () => { | ||
| // Fully different 6000-line texts: past the search cap, the changed middle | ||
| // is every old line removed, then every new line added. | ||
| const lineCount = 6000; | ||
| const oldLines = Array.from({length: lineCount}, (_, i) => `old ${i}`); | ||
| const newLines = Array.from({length: lineCount}, (_, i) => `new ${i}`); | ||
| const file = createDiffFile({ | ||
| oldValue: ['head', ...oldLines, 'tail'].join('\n'), | ||
| newValue: ['head', ...newLines, 'tail'].join('\n'), | ||
| contextLines: 1, | ||
| }); | ||
| expect(file.hunks).toHaveLength(1); | ||
| const [hunk] = file.hunks; | ||
| expect(hunk.header).toBe(`@@ -1,${lineCount + 2} +1,${lineCount + 2} @@`); | ||
| const kinds = hunk.lines.map(line => line.kind); | ||
| expect(kinds.filter(kind => kind === 'removed')).toHaveLength(lineCount); | ||
| expect(kinds.filter(kind => kind === 'added')).toHaveLength(lineCount); | ||
| // Shared lines are still context, and every removal precedes the additions. | ||
| expect(kinds[0]).toBe('context'); | ||
| expect(kinds[kinds.length - 1]).toBe('context'); | ||
| expect(kinds.indexOf('added')).toBe(lineCount + 1); | ||
| const {oldSide, newSide} = sides(file); | ||
| expect(oldSide).toEqual(['head', ...oldLines, 'tail']); | ||
| expect(newSide).toEqual(['head', ...newLines, 'tail']); | ||
| }); | ||
|
|
||
| it('handles a very long text changed only at both ends', () => { | ||
| // Every line in between is matched by the search, so the edit script | ||
| // holds one op per line: more than a call can take as spread arguments. | ||
| const lineCount = 150_000; | ||
| const base = Array.from({length: lineCount}, (_, i) => `line ${i}`); | ||
| const next = [...base]; | ||
| next[0] = 'first changed'; | ||
| next[lineCount - 1] = 'last changed'; | ||
| const file = createDiffFile({ | ||
| oldValue: base.join('\n'), | ||
| newValue: next.join('\n'), | ||
| contextLines: 1, | ||
| }); | ||
| expect(file.hunks.map(h => h.header)).toEqual([ | ||
| '@@ -1,2 +1,2 @@', | ||
| `@@ -${lineCount - 1},2 +${lineCount - 1},2 @@`, | ||
| ]); | ||
| }); | ||
|
|
||
| it('uses the default context when contextLines is NaN', () => { | ||
| const file = createDiffFile({ | ||
| oldValue: 'a\nb\nc', | ||
| newValue: 'a\nB\nc', | ||
| contextLines: NaN, | ||
| }); | ||
| expect(file.hunks.map(h => h.header)).toEqual(['@@ -1,3 +1,3 @@']); | ||
| }); | ||
|
|
||
| it('marks a rename when the paths differ', () => { | ||
| const file = createDiffFile({ | ||
| oldValue: 'a', | ||
| newValue: 'a', | ||
| oldPath: 'a.ts', | ||
| newPath: 'b.ts', | ||
| }); | ||
| expect(file.status).toBe('renamed'); | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: this test also passes with
MAX_EDIT_DISTANCEraised to 100,000, because Myers gives a full rewrite the same all-removed-then-all-added block as the fallback, only more slowly (about 1.3 s instead of 0.07 s here). A case whose minimal and capped results differ would pin the cap, for example: