File mentions enhancements: connection support and performance improvements#132
File mentions enhancements: connection support and performance improvements#132yonikliger-velocity wants to merge 2 commits intomainfrom
Conversation
Greptile SummaryThis PR extends the @ file mention feature to connection sessions by aggregating file indexes from all member worktrees into a single keyed entry, adds improved error handling and a user-facing error banner, and cleans up Key issues found:
Confidence Score: 2/5
Important Files Changed
Last reviewed commit: a3b8b00 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aba256b469
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Previously, file mentions were limited to showing only 5 files when typing @ in the input field. This made it difficult to find files in larger projects, especially in connection sessions with multiple worktrees. Changes to useFileMentions.ts: - Removed .slice(0, 5) limit for both empty and search queries - Added MAX_SUGGESTIONS = 200 cap to prevent UI performance issues - Eliminated double filterSuggestions computation (was called in both suggestions and effectiveSuggestions memos) - Now only effectiveSuggestions memo calls filterSuggestions - moveSelection callback uses effectiveSuggestions.length - Removed verbose debug console.log statements from hot path - All matching files shown with proper filtering and scoring Changes to FileMentionPopover.tsx: - Fixed React key from file.relativePath to file.path - Prevents React confusion when multiple worktrees have same file names - Ensures proper re-rendering when suggestions change across keystrok es Performance: - Handles large file lists (1000+ files) efficiently - Caps results at 200 to maintain UI responsiveness - Single filterSuggestions call per keystroke (not double) - No verbose logging in production hot path - Keyboard navigation (arrow keys) works smoothly with all results - Automatic scroll-to-view for selected items Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Previously, @file mentions did not work in connection sessions because: 1. Connection path resolution failures were silently ignored (console.warn only) 2. Connections aggregate multiple worktrees, not a single git repository 3. git ls-files on connection path would fail (not a git repository) 4. File index was never loaded for connection sessions 5. Connection data wasn't subscribed to, causing timing issues This commit adds proper connection support with comprehensive fixes: SessionView.tsx changes: - Added connectionPathError state to track path resolution failures - Improved error handling with toast notifications and detailed console.error - Added error banner UI with retry button when connection path fails - Enhanced file index loading to detect connection vs worktree sessions - Subscribe to activeConnection via useConnectionStore hook (not imperative getState) - Ensures effect re-runs when connection data loads after initial mount - Removed connectionPathError from effect dependencies to prevent race conditions - Now loads files from all connection member worktrees with symlink prefixes - Passes member data (symlinkName + worktreePath) to file tree store useFileTreeStore.ts changes: - Added loadFileIndexForConnection(connectionId, members[]) method - Accepts members array with { symlinkName, worktreePath } objects - Prefixes each file's relativePath with member symlink name - Example: "repo-a/src/index.ts" distinguishes from "repo-b/src/index.ts" - Prevents ambiguity when multiple worktrees have same relative paths - Loads files from all member worktrees in parallel using Promise.all - Removes duplicate files based on prefixed relativePath (not path) - Uses connection:${id} as cache key to distinguish from worktree sessions - Tracks hasFailures flag to detect when all member scans fail - Doesn't store empty array on total failure (allows retry via reference check) - Ensures fileIndex === EMPTY_FILE_INDEX guard can trigger retry - Added comprehensive error logging for debugging scan failures File mentions now work correctly in both: - Single worktree sessions (unchanged behavior, no prefix) - Connection sessions (new: aggregates from all members with prefixes) User experience improvements: - Clear error messages when connection fails to load - Retry button for failed connections - Toast notifications for failures - File paths disambiguated with member symlink names - Proper retry behavior when connection data loads late - No spurious effect re-runs from connectionPathError state changes Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
yonikliger-velocity
force-pushed
the
annotate-files-enhancements
branch
from
8b500a7 to
a3b8b00
Compare
| } | ||
| }, | ||
|
|
||
| // Load and aggregate file index for a connection from all member worktrees | ||
| loadFileIndexForConnection: async ( | ||
| connectionId: string, | ||
| members: Array<{ symlinkName: string; worktreePath: string }> | ||
| ) => { | ||
| // Use connectionId as the key for the aggregated file list | ||
| const cacheKey = `connection:${connectionId}` | ||
|
|
||
| // Prevent duplicate concurrent loads | ||
| if (get().fileIndexLoadingByWorktree.get(cacheKey)) return | ||
|
|
||
| set((state) => { | ||
| const newMap = new Map(state.fileIndexLoadingByWorktree) | ||
| newMap.set(cacheKey, true) | ||
| return { fileIndexLoadingByWorktree: newMap } | ||
| }) | ||
|
|
||
| try { | ||
| // Load files from all member worktrees in parallel | ||
| const filePromises = members.map((member) => | ||
| window.fileTreeOps.scanFlat(member.worktreePath).then((result) => ({ | ||
| member, | ||
| result | ||
| })) | ||
| ) | ||
| const results = await Promise.all(filePromises) | ||
|
|
||
| // Aggregate all files from all member worktrees | ||
| // Prefix each file's relativePath with the member's symlink name | ||
| const aggregatedFiles: FlatFile[] = [] | ||
| let hasFailures = false | ||
|
|
||
| results.forEach(({ member, result }) => { | ||
| if (result.success && result.files) { | ||
| // Prefix files with symlink name to disambiguate files with same names | ||
| const prefixedFiles = result.files.map((file) => ({ | ||
| ...file, | ||
| relativePath: `${member.symlinkName}/${file.relativePath}` | ||
| })) | ||
| aggregatedFiles.push(...prefixedFiles) | ||
| } else { | ||
| hasFailures = true | ||
| } | ||
| }) | ||
|
|
||
| // Only store the index if we got at least some results | ||
| // If all scans failed and aggregatedFiles is empty, don't store it | ||
| // so the reference-equality guard (fileIndex === EMPTY_FILE_INDEX) will retry | ||
| if (aggregatedFiles.length === 0 && hasFailures) { | ||
| console.error( | ||
| `[FileTreeStore] All member scans failed for connection ${connectionId} - not storing empty index` | ||
| ) | ||
| set((state) => { | ||
| const loadingMap = new Map(state.fileIndexLoadingByWorktree) | ||
| loadingMap.set(cacheKey, false) | ||
| return { fileIndexLoadingByWorktree: loadingMap } | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| // Remove duplicates based on prefixed relativePath | ||
| const uniqueFiles = Array.from( | ||
| new Map(aggregatedFiles.map((f) => [f.relativePath, f])).values() | ||
| ) | ||
|
|
||
| set((state) => { | ||
| const indexMap = new Map(state.fileIndexByWorktree) | ||
| const loadingMap = new Map(state.fileIndexLoadingByWorktree) | ||
| indexMap.set(cacheKey, uniqueFiles) | ||
| loadingMap.set(cacheKey, false) | ||
| return { fileIndexByWorktree: indexMap, fileIndexLoadingByWorktree: loadingMap } | ||
| }) | ||
|
|
||
| console.log( | ||
| `[FileTreeStore] Loaded ${uniqueFiles.length} files for connection ${connectionId} from ${members.length} members` | ||
| ) | ||
| } catch (error) { | ||
| console.error('[FileTreeStore] Exception loading connection files:', error, 'connectionId:', connectionId) | ||
| set((state) => { | ||
| const loadingMap = new Map(state.fileIndexLoadingByWorktree) | ||
| loadingMap.set(cacheKey, false) | ||
| return { fileIndexLoadingByWorktree: loadingMap } | ||
| }) | ||
| } | ||
| }, |
There was a problem hiding this comment.
Connection file index never stays fresh — no file watchers set up
loadFileIndexForConnection loads the aggregated index once but never subscribes to file-system changes. Compare with loadFileIndex, which calls get().startWatching(worktreePath) after loading — that subscription is what keeps the regular file index up to date as files are added/deleted.
Without calling startWatching for each member worktree, any file added, renamed, or deleted within a connection member's directory is invisible to the @ mention list until the user triggers a full session reload. For a "connection" feature that aggregates multiple active repos, this is likely to cause persistent confusion ("why isn't my new file showing up?").
Each member worktree should be watched, and the handleFileChange callback should map changes back to a re-scan/update of the aggregated connection:${connectionId} index:
// After the set() call that stores uniqueFiles…
for (const member of members) {
get().startWatching(member.worktreePath)
}Note that handleFileChange currently operates on a single worktreePath key and does incremental updates; for the connection case you'd either need to extend it or do a full re-scan (loadFileIndexForConnection) on any member change event.
| results.forEach(({ member, result }) => { | ||
| if (result.success && result.files) { | ||
| // Prefix files with symlink name to disambiguate files with same names | ||
| const prefixedFiles = result.files.map((file) => ({ | ||
| ...file, | ||
| relativePath: `${member.symlinkName}/${file.relativePath}` | ||
| })) | ||
| aggregatedFiles.push(...prefixedFiles) | ||
| } else { | ||
| hasFailures = true | ||
| } | ||
| }) | ||
|
|
||
| // Only store the index if we got at least some results | ||
| // If all scans failed and aggregatedFiles is empty, don't store it | ||
| // so the reference-equality guard (fileIndex === EMPTY_FILE_INDEX) will retry | ||
| if (aggregatedFiles.length === 0 && hasFailures) { | ||
| console.error( | ||
| `[FileTreeStore] All member scans failed for connection ${connectionId} - not storing empty index` | ||
| ) | ||
| set((state) => { | ||
| const loadingMap = new Map(state.fileIndexLoadingByWorktree) | ||
| loadingMap.set(cacheKey, false) | ||
| return { fileIndexLoadingByWorktree: loadingMap } | ||
| }) | ||
| return |
There was a problem hiding this comment.
Partial member scan failures stored silently with no user feedback
hasFailures is only consulted to avoid writing a completely empty index (the aggregatedFiles.length === 0 && hasFailures guard). When at least one member succeeds but others fail, hasFailures is true yet execution falls through to store the partial result without any log or UI signal.
This means a connection with three member repos where one is unreachable will silently show only two-thirds of the expected files — the user has no way to know some members were skipped.
Consider logging a warning and/or returning a flag that the caller can use to surface an error banner:
if (hasFailures) {
const failedCount = results.filter(({ result }) => !result.success).length
console.warn(
`[FileTreeStore] ${failedCount}/${members.length} member scans failed for connection ${connectionId} — index is partial`
)
}Returning the number of failures from this function (or accepting a callback) would also let SessionView display a partial-load warning alongside the existing connectionPathError banner.
| } else if (!activeConnection) { | ||
| console.warn('[SessionView] Connection not found for file index loading', { | ||
| sessionId, | ||
| connectionId | ||
| }) | ||
| } else { | ||
| console.warn('[SessionView] Connection has no members - file mentions disabled', { | ||
| sessionId, | ||
| connectionId | ||
| }) | ||
| } |
There was a problem hiding this comment.
Zero-member connection causes repeated console warnings on every effect re-run
When activeConnection.members.length === 0, the effect logs a warning and returns without storing anything. Because fileIndex === EMPTY_FILE_INDEX remains true, any subsequent change to an unrelated dependency (e.g. the activeConnection reference itself being replaced by the store on an unrelated update) will re-trigger the effect and log the same warning repeatedly.
Consider storing a sentinel value or a local ref to record that the zero-members case has already been handled, so the warning fires at most once per session mount:
const zeroMembersWarnedRef = useRef(false)
// …inside the else branch:
if (!zeroMembersWarnedRef.current) {
console.warn('[SessionView] Connection has no members - file mentions disabled', …)
zeroMembersWarnedRef.current = true
}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3b8b00ca2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Regular worktree session - load from single worktree | ||
| console.log('[SessionView] Loading file index for worktree:', worktreePath) | ||
| useFileTreeStore.getState().loadFileIndex(worktreePath) | ||
| } else if (connectionId && fileIndex === EMPTY_FILE_INDEX) { |
There was a problem hiding this comment.
The connection branch only calls loadFileIndexForConnection while fileIndex === EMPTY_FILE_INDEX, so once a connection index is cached it is never refreshed even if activeConnection.members changes (for example via addMember/removeMember/updateConnectionMembers in useConnectionStore). In an active connection session this leaves @ suggestions stale (new members' files never appear, removed members' files remain) until the view is remounted or cache is manually cleared.
Useful? React with 👍 / 👎.
1 participant
Summary
This PR enhances the file mentions (@) feature with support for connection sessions and significant performance improvements for large repositories.
Key Changes
Connection Session Support
loadFileIndexForConnection()to aggregate files from all member worktrees in a connectionconnection:${connectionId}as cache key for connection sessionsPerformance Optimizations
Bug Fixes
FileMentionPopoverto usepathinstead ofrelativePathfor consistencyTechnical Details
Files Changed
Testing Recommendations
Impact
🤖 Generated with Claude Code