feat: @ syntax for bringing files into chat context by weilirs · Pull Request #475 · reorproject/reor

weilirs · 2024-11-04T10:17:14Z

Updated version
https://github.com/user-attachments/assets/522bfd40-20dc-43e1-822c-5098024cc99f

greptile-apps

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 searchFiles function as it exposes file paths without proper validation or sanitization in electron/main/filesystem/filesystem.ts
File search implementation in searchFiles loads all files into memory before filtering, which could cause performance issues with large directories
Regex pattern /@([^@]+?\.md)/g in extractFileReferences is fragile and only handles .md files without validating existence
Caret position calculation in getCaretCoordinates creates/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}

greptile-apps · 2024-11-04T10:17:54Z

  return { localModelPath, repoName }
 }
+
+export const searchFiles = async (store: Store<StoreSchema>, searchTerm: string): Promise<string[]> => {


style: Function marked as async but contains no await operations

greptile-apps · 2024-11-04T10:17:55Z

 }
+
+export const searchFiles = async (store: Store<StoreSchema>, searchTerm: string): Promise<string[]> => {
+  const vaultDirectory = store.get(StoreKeys.DirectoryFromPreviousSession)


logic: Using DirectoryFromPreviousSession instead of current window's vault directory could cause inconsistencies

greptile-apps · 2024-11-04T10:17:55Z

+  const allFiles = GetFilesInfoList(vaultDirectory)
+
+  const searchTermLower = searchTerm.toLowerCase()
+
+  const matchingFiles = allFiles.filter((file) => file.name.toLowerCase().startsWith(searchTermLower))


style: Loading all files into memory before filtering could be inefficient for large vaults. Consider using a streaming/iterative approach

greptile-apps · 2024-11-04T10:17:56Z

+  if (!vaultDirectory || typeof vaultDirectory !== 'string') {
+    throw new Error('No valid vault directory found')
+  }


style: Error handling could provide more context about why the vault directory is invalid

greptile-apps · 2024-11-04T10:18:28Z

+  ipcMain.handle('search-files', async (_event, searchTerm: string) => {
+    return searchFiles(store, searchTerm)
+  })


logic: No error handling for searchFiles - could throw unhandled exceptions if store is invalid or searchTerm is malformed. Consider wrapping in try/catch.

greptile-apps · 2024-11-04T10:20:05Z

+    >
+      {files.map((file) => (
+        <div key={file} className="cursor-pointer px-4 py-2 hover:bg-neutral-700" onClick={() => onSelect(file)}>
+          {file.split('/').pop()}


logic: file.split('/').pop() could return undefined if path ends in /

greptile-apps · 2024-11-04T10:20:30Z

+  const handleFileSelect = (filePath: string) => {
+    const lastAtIndex = userTextFieldInput.lastIndexOf('@')
+    const newValue = `${userTextFieldInput.slice(0, lastAtIndex)}@${filePath} ${userTextFieldInput.slice(
+      lastAtIndex + searchTerm.length + 1,
+    )}`


logic: Potential XSS vulnerability - filePath needs to be sanitized before being inserted into the text input

greptile-apps · 2024-11-04T10:20:31Z

+    // 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)


logic: Memory leak risk - mirror element needs cleanup if component unmounts while calculating coordinates

greptile-apps · 2024-11-04T10:20:58Z

+  const regex = /@([^@]+?\.md)/g
+  const matches = message.match(regex)


logic: regex pattern only matches .md files and doesn't handle spaces or special characters in filenames

greptile-apps · 2024-11-04T10:20:58Z

+        const fileRefs = extractFileReferences(userTextFieldInput || '')
+        // console.log('fileRefs', fileRefs)


logic: no validation that referenced files exist before adding to chat context

milaiwi · 2024-11-04T19:37:01Z

Will take a look tonight

Edit: Delayed due to comment below.

samlhuillier · 2024-11-04T20:36:44Z

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

weilirs · 2024-11-05T01:56:06Z

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.

greptile-apps

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 FileAutocomplete component in /src/components/Chat/FileAutocomplete.tsx for handling file selection UI with hover previews
Implemented ChatSources component in /src/components/Chat/MessageComponents/ChatSources.tsx to display referenced files with preview functionality
Added vault path resolution in extractFileReferences function 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 FileAutocomplete doesn't account for different operating systems' path separators
Autocomplete positioning logic may break with scrolling/resizing
Missing loading/empty states in FileAutocomplete component
No debouncing on file search operations which could impact performance

_{9 file(s) reviewed, 14 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-11-20T08:59:57Z

+  const matchingFiles = allFiles
+    .filter((file) => file.name.toLowerCase().startsWith(searchTermLower))


logic: Case-sensitive path handling could cause issues on Windows. Consider using case-insensitive comparison with localeCompare

greptile-apps · 2024-11-20T08:59:57Z

+    .filter((file) => file.name.toLowerCase().startsWith(searchTermLower))
+    .map((file) => ({
+      absolutePath: file.path,
+      relativePath: file.path.replace(vaultDirectory, '').replace(/^[/\\]+/, ''),


logic: Path separator handling with replace(/^[/\]+/, '') may not work correctly on Windows. Use path.normalize() instead

greptile-apps · 2024-11-20T09:00:24Z

+  ipcMain.handle('get-vault-path', async () => {
+    return getVaultPath(store)
+  })


logic: No error handling for getVaultPath - could expose system errors to renderer process

greptile-apps · 2024-11-20T09:00:46Z

+  searchFiles:
+    createIPCHandler<(searchTerm: string) => Promise<Array<{ absolutePath: string; relativePath: string }>>>(
+      'search-files',
+    ),


logic: Consider adding input validation/sanitization for searchTerm to prevent path traversal attacks

greptile-apps · 2024-11-20T09:00:47Z

  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 }>>>(


style: The return type should be more specific - consider creating an interface for the path object structure

greptile-apps · 2024-11-20T09:01:49Z

+      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,
+      }}


style: absolute positioning with fixed width could cause overflow issues on small screens. Consider making width responsive or adding overflow handling

greptile-apps · 2024-11-20T09:01:49Z

+        left: position.left,
+      }}
+    >
+      {files.map((file) => {


style: consider limiting number of results shown to prevent performance issues with large file lists

greptile-apps · 2024-11-20T09:02:30Z

+  const vaultPath = await window.fileSystem.getVaultPath()
+  return matches.map((match) => {
+    const relativePath = match.slice(1) // Remove @ symbol
+
+    return `${vaultPath}/${relativePath}`
+  })


logic: path concatenation with string interpolation is unsafe and may cause path traversal vulnerabilities. Use path.join() instead

greptile-apps · 2024-11-20T09:02:31Z

          return
        }

+        const fileRefs = await extractFileReferences(userTextFieldInput || '')


style: consider adding error handling for when getVaultPath() fails

greptile-apps · 2024-11-20T09:02:49Z

  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


style: Using window.confirm is not ideal for modern UX. Consider using a modal dialog component instead.

feat: @ syntax draft

f62afac

weilirs requested review from joseplayero, milaiwi and samlhuillier November 4, 2024 10:17

greptile-apps Bot reviewed Nov 4, 2024

View reviewed changes

weilirs marked this pull request as draft November 5, 2024 01:54

weilirs added 5 commits November 14, 2024 11:18

Merge remote-tracking branch 'upstream/main' into @-syntax

5f60dee

fix: adjust autocomplete menu's position

8ea3d57

folder information in autocomplete

a559036

relative path in chat

b13d21e

clean up

d5f509a

weilirs marked this pull request as ready for review November 20, 2024 08:59

greptile-apps Bot reviewed Nov 20, 2024

View reviewed changes

		const regex = /@([^@]+?\.md)/g
		const matches = message.match(regex)

		const fileRefs = extractFileReferences(userTextFieldInput \|\| '')
		// console.log('fileRefs', fileRefs)

		const matchingFiles = allFiles
		.filter((file) => file.name.toLowerCase().startsWith(searchTermLower))

Uh oh!

Conversation

weilirs commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

PR Summary

Uh oh!

greptile-apps Bot Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

milaiwi commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samlhuillier commented Nov 4, 2024

Uh oh!

weilirs commented Nov 5, 2024

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

PR Summary

Uh oh!

greptile-apps Bot Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

weilirs commented Nov 4, 2024 •

edited

Loading

milaiwi commented Nov 4, 2024 •

edited

Loading