feat: @ syntax for bringing files into chat context #475
Conversation
![greptile-apps[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Added @ syntax functionality for referencing files in chat context, enabling users to search and include files in conversations through an autocomplete interface.
- Potential security risk in
searchFilesfunction as it exposes file paths without proper validation or sanitization inelectron/main/filesystem/filesystem.ts - File search implementation in
searchFilesloads all files into memory before filtering, which could cause performance issues with large directories - Regex pattern
/@([^@]+?\.md)/ginextractFileReferencesis fragile and only handles .md files without validating existence - Caret position calculation in
getCaretCoordinatescreates/removes DOM elements frequently without debouncing - Missing error handling for file operations and IPC calls across multiple components
7 file(s) reviewed, 17 comment(s)
Edit PR Review Bot Settings | Greptile
| return { localModelPath, repoName } | ||
| } | ||
|
|
||
| export const searchFiles = async (store: Store<StoreSchema>, searchTerm: string): Promise<string[]> => { |
There was a problem hiding this comment.
style: Function marked as async but contains no await operations
| } | ||
|
|
||
| export const searchFiles = async (store: Store<StoreSchema>, searchTerm: string): Promise<string[]> => { | ||
| const vaultDirectory = store.get(StoreKeys.DirectoryFromPreviousSession) |
There was a problem hiding this comment.
logic: Using DirectoryFromPreviousSession instead of current window's vault directory could cause inconsistencies
| const allFiles = GetFilesInfoList(vaultDirectory) | ||
|
|
||
| const searchTermLower = searchTerm.toLowerCase() | ||
|
|
||
| const matchingFiles = allFiles.filter((file) => file.name.toLowerCase().startsWith(searchTermLower)) |
There was a problem hiding this comment.
style: Loading all files into memory before filtering could be inefficient for large vaults. Consider using a streaming/iterative approach
| if (!vaultDirectory || typeof vaultDirectory !== 'string') { | ||
| throw new Error('No valid vault directory found') | ||
| } |
There was a problem hiding this comment.
style: Error handling could provide more context about why the vault directory is invalid
| ipcMain.handle('search-files', async (_event, searchTerm: string) => { | ||
| return searchFiles(store, searchTerm) | ||
| }) |
There was a problem hiding this comment.
logic: No error handling for searchFiles - could throw unhandled exceptions if store is invalid or searchTerm is malformed. Consider wrapping in try/catch.
| > | ||
| {files.map((file) => ( | ||
| <div key={file} className="cursor-pointer px-4 py-2 hover:bg-neutral-700" onClick={() => onSelect(file)}> | ||
| {file.split('/').pop()} |
There was a problem hiding this comment.
logic: file.split('/').pop() could return undefined if path ends in /
src/components/Chat/StartChat.tsx
Outdated
| const handleFileSelect = (filePath: string) => { | ||
| const lastAtIndex = userTextFieldInput.lastIndexOf('@') | ||
| const newValue = `${userTextFieldInput.slice(0, lastAtIndex)}@${filePath} ${userTextFieldInput.slice( | ||
| lastAtIndex + searchTerm.length + 1, | ||
| )}` |
There was a problem hiding this comment.
logic: Potential XSS vulnerability - filePath needs to be sanitized before being inserted into the text input
src/components/Chat/StartChat.tsx
Outdated
| // Create a hidden div with the same styling as textarea | ||
| const mirror = document.createElement('div') | ||
| mirror.style.cssText = window.getComputedStyle(element).cssText | ||
| mirror.style.height = 'auto' | ||
| mirror.style.position = 'absolute' | ||
| mirror.style.visibility = 'hidden' | ||
| mirror.style.whiteSpace = 'pre-wrap' | ||
| document.body.appendChild(mirror) |
There was a problem hiding this comment.
logic: Memory leak risk - mirror element needs cleanup if component unmounts while calculating coordinates
| const regex = /@([^@]+?\.md)/g | ||
| const matches = message.match(regex) |
There was a problem hiding this comment.
logic: regex pattern only matches .md files and doesn't handle spaces or special characters in filenames
src/components/Chat/index.tsx
Outdated
| const fileRefs = extractFileReferences(userTextFieldInput || '') | ||
| // console.log('fileRefs', fileRefs) |
There was a problem hiding this comment.
logic: no validation that referenced files exist before adding to chat context
|
Will take a look tonight Edit: Delayed due to comment below. |
|
I'm afraid we'll need to hold off on merging this PR. I'm doing a bit of a revamp of chat to get it working nicely in the sidebar #472 so this feature can only be merged after that's done |
Okay, this PR needs more work as well, just want some opinions from you guys. |
There was a problem hiding this comment.
PR Summary
(updates since last review)
This PR continues to evolve the @ syntax feature for file references in chat context, with several new changes and components. Here's what's new:
- Added
FileAutocompletecomponent in/src/components/Chat/FileAutocomplete.tsxfor handling file selection UI with hover previews - Implemented
ChatSourcescomponent in/src/components/Chat/MessageComponents/ChatSources.tsxto display referenced files with preview functionality - Added vault path resolution in
extractFileReferencesfunction to convert relative paths to absolute paths - Introduced analytics tracking for LLM-related actions in
DefaultLLMSelector.tsx
The implementation still needs attention in these areas:
- File path handling in
FileAutocompletedoesn't account for different operating systems' path separators - Autocomplete positioning logic may break with scrolling/resizing
- Missing loading/empty states in
FileAutocompletecomponent - No debouncing on file search operations which could impact performance
9 file(s) reviewed, 14 comment(s)
Edit PR Review Bot Settings | Greptile
| const matchingFiles = allFiles | ||
| .filter((file) => file.name.toLowerCase().startsWith(searchTermLower)) |
There was a problem hiding this comment.
logic: Case-sensitive path handling could cause issues on Windows. Consider using case-insensitive comparison with localeCompare
| .filter((file) => file.name.toLowerCase().startsWith(searchTermLower)) | ||
| .map((file) => ({ | ||
| absolutePath: file.path, | ||
| relativePath: file.path.replace(vaultDirectory, '').replace(/^[/\\]+/, ''), |
There was a problem hiding this comment.
logic: Path separator handling with replace(/^[/\]+/, '') may not work correctly on Windows. Use path.normalize() instead
| ipcMain.handle('get-vault-path', async () => { | ||
| return getVaultPath(store) | ||
| }) |
There was a problem hiding this comment.
logic: No error handling for getVaultPath - could expose system errors to renderer process
| searchFiles: | ||
| createIPCHandler<(searchTerm: string) => Promise<Array<{ absolutePath: string; relativePath: string }>>>( | ||
| 'search-files', | ||
| ), |
There was a problem hiding this comment.
logic: Consider adding input validation/sanitization for searchTerm to prevent path traversal attacks
| getAllFilenamesInDirectory: createIPCHandler<(dirName: string) => Promise<string[]>>('get-files-in-directory'), | ||
| getFiles: createIPCHandler<(filePaths: string[]) => Promise<FileInfoWithContent[]>>('get-files'), | ||
| searchFiles: | ||
| createIPCHandler<(searchTerm: string) => Promise<Array<{ absolutePath: string; relativePath: string }>>>( |
There was a problem hiding this comment.
style: The return type should be more specific - consider creating an interface for the path object structure
| className="absolute z-50 max-h-48 w-64 overflow-y-auto rounded-md border border-neutral-700 bg-background shadow-lg" | ||
| style={{ | ||
| top: 'auto', | ||
| bottom: `calc(100% + 10px)`, | ||
| left: position.left, | ||
| }} |
There was a problem hiding this comment.
style: absolute positioning with fixed width could cause overflow issues on small screens. Consider making width responsive or adding overflow handling
| left: position.left, | ||
| }} | ||
| > | ||
| {files.map((file) => { |
There was a problem hiding this comment.
style: consider limiting number of results shown to prevent performance issues with large file lists
| const vaultPath = await window.fileSystem.getVaultPath() | ||
| return matches.map((match) => { | ||
| const relativePath = match.slice(1) // Remove @ symbol | ||
|
|
||
| return `${vaultPath}/${relativePath}` | ||
| }) |
There was a problem hiding this comment.
logic: path concatenation with string interpolation is unsafe and may cause path traversal vulnerabilities. Use path.join() instead
| return | ||
| } | ||
|
|
||
| const fileRefs = await extractFileReferences(userTextFieldInput || '') |
There was a problem hiding this comment.
style: consider adding error handling for when getVaultPath() fails
| const handleDeleteLLM = async (modelName: string) => { | ||
| // eslint-disable-next-line no-alert | ||
| const confirmDelete = window.confirm(`Are you sure you want to delete the model ${modelName}?`) | ||
| if (!confirmDelete) return |
There was a problem hiding this comment.
style: Using window.confirm is not ideal for modern UX. Consider using a modal dialog component instead.
3 participants
#467
Updated version
https://github.com/user-attachments/assets/522bfd40-20dc-43e1-822c-5098024cc99f