feat: @ syntax for bringing files into chat context #475
Conversation
![greptile-apps[bot]](https://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
| @@ -217,3 +219,18 @@ export function splitDirectoryPathIntoBaseAndRepo(fullPath: string) { | |||
|
|
|||
| 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
| @@ -217,3 +219,18 @@ export function splitDirectoryPathIntoBaseAndRepo(fullPath: string) { | |||
|
|
|||
| return { localModelPath, repoName } | |||
| } | |||
|
|
|||
| 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 /
| 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
| // 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
| 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. |
#467
If this looks good I'll
@.syntax.mov