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(vscode): add at feature with AT file implementation in VSCode #3602

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Sma1lboy
Copy link
Collaborator

@Sma1lboy Sma1lboy commented Dec 20, 2024

not git repo

image

in git repo

image

@Sma1lboy Sma1lboy force-pushed the feat-at-function-vscode branch from 5244117 to ccad4a3 Compare January 6, 2025 06:28
@Sma1lboy Sma1lboy changed the title feat(vscode): add at functions implementation in vscode feat(vscode): add at feature with AT file implementation in VSCode Jan 6, 2025
Comment on lines +228 to +239
let label: string;

if (filepath.kind === "git") {
label = filepath.filepath;
} else {
const workspaceFolder = workspace.getWorkspaceFolder(uri);
if (workspaceFolder) {
label = path.relative(workspaceFolder.uri.fsPath, uri.fsPath);
} else {
label = path.basename(uri.fsPath);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Please try to use method workspace.asRelativePath:

Suggested change
let label: string;
if (filepath.kind === "git") {
label = filepath.filepath;
} else {
const workspaceFolder = workspace.getWorkspaceFolder(uri);
if (workspaceFolder) {
label = path.relative(workspaceFolder.uri.fsPath, uri.fsPath);
} else {
label = path.basename(uri.fsPath);
}
}
const label = worksapce.asRelativePath(uri);

Comment on lines +247 to +270
export function extractTextFromRange(document: TextDocument, range: LineRange | PositionRange): string {
if (typeof range.start === "number" && typeof range.end === "number") {
const startLine = range.start - 1;
const endLine = range.end - 1;
let selectedText = "";

for (let i = startLine; i <= endLine; i++) {
if (i < 0 || i >= document.lineCount) {
continue;
}
selectedText += document.lineAt(i).text + "\n";
}
return selectedText;
}

if (typeof range.start === "object" && typeof range.end === "object") {
const startPos = new VSCodePosition(range.start.line - 1, range.start.character - 1);
const endPos = new VSCodePosition(range.end.line - 1, range.end.character - 1);
const selectedRange = new VSCodeRange(startPos, endPos);
return document.getText(selectedRange);
}

return "";
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we reuse chatPanelLocationToVSCodeRange to convert range: LineRange | PositionRange to VSCodeRange?
By reusing thiat, there is no need to extract extractTextFromRange.

this.logger.info(`No query provided, listing ${openTabs.length} opened editors.`);

return openTabs
.filter((tab) => tab.uri.scheme === "file")
Copy link
Member

Choose a reason for hiding this comment

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

Why is this filter required?

Comment on lines +475 to +478
.map((tab) => ({
uri: (tab.input as TabInputText).uri,
label: tab.label,
})),
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 this map is not required here. You can directly map it with uriToListFileItem.

.map((tab) => uriToListFileItem(tab.uri, this.gitProvider));
}

const globPattern = `**/${query}*`;
Copy link
Member

Choose a reason for hiding this comment

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

Should we validate the query string before injecting it into the glob pattern to ensure **/${query}* is safe? For example, what if the query string contains * or other special characters?

},

readFileContent: async (info: FileRange): Promise<string | null> => {
if (info.range) {
Copy link
Member

Choose a reason for hiding this comment

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

There's no need for an if branch here. document.getText() can accept a range or undefined as parameters.

Comment on lines +511 to +514
} catch (error) {
this.logger.error("Failed to get file content by range:", error);
return null;
}
Copy link
Member

Choose a reason for hiding this comment

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

No need to catch error, or just rethrow it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants