Skip to content
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

feat(agent): add removeDuplicateSuffixLines postprocess filter #3392

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion clients/tabby-agent/src/codeCompletion/postprocess/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { trimMultiLineInSingleLineMode } from "./trimMultiLineInSingleLineMode";
import { dropDuplicated } from "./dropDuplicated";
import { dropMinimum } from "./dropMinimum";
import { calculateReplaceRange } from "./calculateReplaceRange";
import { removeDuplicateSuffixLines } from "./removeDuplicateSuffixLines";

type ItemListFilter = (items: CompletionItem[]) => Promise<CompletionItem[]>;

Expand Down Expand Up @@ -54,5 +55,6 @@ export async function postCacheProcess(
.then(applyFilter(dropDuplicated))
.then(applyFilter(trimSpace))
.then(applyFilter(dropMinimum))
.then(applyFilter(calculateReplaceRange));
.then(applyFilter(calculateReplaceRange))
.then(applyFilter(removeDuplicateSuffixLines));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be added before dropMinimum

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
import { documentContext, inline, assertFilterResult } from "./testUtils";
import { removeDuplicateSuffixLines } from "./removeDuplicateSuffixLines";

describe("postprocess", () => {
describe("removeDuplicateSuffixLines", () => {
const filter = removeDuplicateSuffixLines();

it("should remove duplicated suffix lines", async () => {
const context = documentContext`
function example() {
const items = [
];
}
`;
Comment on lines +9 to +15
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const context = documentContext`
function example() {
const items = [
];
}
`;
const context = documentContext`
function example() {
const items = [
4,
5,
6
];
}
`;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add suffix in documentContext
Please also update code for other test cases.

context.language = "javascript";
const completion = inline`
├1,
2,
3,
4,┤
`;
context.suffix = `
4,
5,
6
];
}
`;
Comment on lines +23 to +29
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
context.suffix = `
4,
5,
6
];
}
`;

const expected = inline`
├1,
2,
3,┤
`;
await assertFilterResult(filter, context, completion, expected);
});

it("should handle empty suffix", async () => {
const context = documentContext`
const value = ║
`;
context.language = "javascript";
const completion = inline`
├42;┤
`;
context.suffix = "";
const expected = completion;
await assertFilterResult(filter, context, completion, expected);
});

it("should handle multiple line matches", async () => {
const context = documentContext`
class Example {
constructor() {
}
}
`;
context.language = "javascript";
const completion = inline`
├this.value = 1;
this.name = "test";
this.items = [];
this.setup();┤
`;
context.suffix = `
this.setup();
this.init();
}
}
`;
const expected = inline`
├this.value = 1;
this.name = "test";
this.items = [];┤
`;
await assertFilterResult(filter, context, completion, expected);
});

it("should handle partial line matches without trimming", async () => {
const context = documentContext`
const config = {
};
`;
context.language = "javascript";
const completion = inline`
├name: "test",
value: 42,
items: [],
enabled: true,┤
`;
context.suffix = `
enabled: true,
debug: false
};
`;
const expected = inline`
├name: "test",
value: 42,
items: [],┤
`;
await assertFilterResult(filter, context, completion, expected);
});

it("should not modify when no matches found", async () => {
const context = documentContext`
function process() {
}
`;
context.language = "javascript";
const completion = inline`
├console.log("processing");
return true;┤
`;
context.suffix = `
console.log("done");
}
`;
const expected = completion;
await assertFilterResult(filter, context, completion, expected);
});

it("should handle whitespace differences", async () => {
const context = documentContext`
const arr = [
];
`;
context.language = "javascript";
const completion = inline`
├1,
2,
3,
4,┤
`;
context.suffix = `
4,
5,
6
];
`;
const expected = inline`
├1,
2,
3,┤
`;
await assertFilterResult(filter, context, completion, expected);
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import { PostprocessFilter } from "./base";
import { CompletionItem } from "../solution";
import { isBlank } from "../../utils/string";
import { getLogger } from "../../logger";

export function removeDuplicateSuffixLines(): PostprocessFilter {
return (item: CompletionItem): CompletionItem => {
const log = getLogger("removeDuplicateSuffixLines");
log.info("Processing item" + JSON.stringify(item?.text || ""));

const text = item?.text;
const suffix = item?.context?.suffix;

if (text == null || suffix == null) {
return item;
}
Comment on lines +11 to +16
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use item.lines and context.suffixLines instead of splitting text.


const originalLines = text.split("\n").map((line) => line || "");
const trimmedLines = originalLines.map((line) => (line || "").trim());

const suffixLines = (suffix || "")
.split("\n")
.map((line) => (line || "").trim())
.filter((line) => !isBlank(line));

if (suffixLines.length === 0) {
return item;
}

const firstSuffixLine = suffixLines[0] || "";

// iterate through lines from end to find potential match
for (let i = trimmedLines.length - 1; i >= 0; i--) {
const currentLine = trimmedLines[i] || "";
if (!isBlank(currentLine) && currentLine === firstSuffixLine) {
// check if subsequent lines also match with suffix
let isFullMatch = true;
for (let j = 0; j < suffixLines.length && i + j < trimmedLines.length; j++) {
const suffixLine = suffixLines[j] || "";
const textLine = trimmedLines[i + j] || "";

if (suffixLine !== textLine) {
isFullMatch = false;
break;
}
}

// if all checked lines match, check for code structure
if (isFullMatch) {
const remainingLines = originalLines.slice(0, i);
const lastLine = remainingLines[remainingLines.length - 1] || "";

// skip empty last lines
if (isBlank(lastLine.trim())) {
return item;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why we do not handle the case of an empty last line. Could you give an example?
By the way, it would be good to add a unit test case for each if branch.


// preserve code block structure
if (lastLine.includes("{") || currentLine.includes("}")) {
return item;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also give an example for this case?
As { and } are language-related, please avoid processing like this if possible, and add a guard check for context.language if necessary.


return item.withText(remainingLines.join("\n"));
}
}
}

return item;
};
}
Loading