-
Notifications
You must be signed in to change notification settings - Fork 36
File mentions enhancements: connection support and performance improvements #132
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -459,21 +459,65 @@ export function SessionView({ sessionId }: SessionViewProps): React.JSX.Element | |
| const inputValueRef = useRef('') | ||
| const draftTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null) | ||
|
|
||
| // Flat file index for file mentions and search — keyed by worktree path. | ||
| // Uses git ls-files for a complete, gitignore-respecting file list. | ||
| // Ensure the index is loaded when worktreePath is resolved — SessionView cannot | ||
| // rely on the FileTree sidebar component having already populated the store | ||
| // (sidebar may be collapsed, on a different tab, or targeting a different worktree). | ||
| const fileIndex = useFileTreeStore((state) => | ||
| worktreePath | ||
| ? (state.fileIndexByWorktree.get(worktreePath) ?? EMPTY_FILE_INDEX) | ||
| : EMPTY_FILE_INDEX | ||
| // Connection path resolution error state | ||
| const [connectionPathError, setConnectionPathError] = useState<string | null>(null) | ||
|
|
||
| // Flat file index for file mentions and search. | ||
| // For worktree sessions: use worktreePath as key | ||
| // For connection sessions: use `connection:${connectionId}` as key and aggregate from all members | ||
| const fileIndex = useFileTreeStore((state) => { | ||
| if (worktreeId) { | ||
| // Regular worktree session | ||
| return worktreePath | ||
| ? (state.fileIndexByWorktree.get(worktreePath) ?? EMPTY_FILE_INDEX) | ||
| : EMPTY_FILE_INDEX | ||
| } else if (connectionId) { | ||
| // Connection session - use connectionId as key | ||
| const cacheKey = `connection:${connectionId}` | ||
| return state.fileIndexByWorktree.get(cacheKey) ?? EMPTY_FILE_INDEX | ||
| } | ||
| return EMPTY_FILE_INDEX | ||
| }) | ||
|
|
||
| // Subscribe to connection data for connection sessions | ||
| const activeConnection = useConnectionStore((state) => | ||
| connectionId ? state.connections.find((c) => c.id === connectionId) : undefined | ||
| ) | ||
|
|
||
| useEffect(() => { | ||
| if (worktreePath && fileIndex === EMPTY_FILE_INDEX) { | ||
| if (worktreeId && worktreePath && fileIndex === EMPTY_FILE_INDEX) { | ||
| // 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) { | ||
| // Connection session - load from all member worktrees | ||
| // Use the subscribed activeConnection instead of imperative getState() | ||
| if (activeConnection && activeConnection.members.length > 0) { | ||
| const members = activeConnection.members.map((m) => ({ | ||
| symlinkName: m.symlink_name, | ||
| worktreePath: m.worktree_path | ||
| })) | ||
| console.log( | ||
| '[SessionView] Loading file index for connection:', | ||
| connectionId, | ||
| 'from', | ||
| members.length, | ||
| 'members' | ||
| ) | ||
| useFileTreeStore.getState().loadFileIndexForConnection(connectionId, members) | ||
| } 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 | ||
| }) | ||
| } | ||
|
Comment on lines
+508
to
+518
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Zero-member connection causes repeated console warnings on every effect re-run When Consider storing a sentinel value or a local const zeroMembersWarnedRef = useRef(false)
// …inside the else branch:
if (!zeroMembersWarnedRef.current) {
console.warn('[SessionView] Connection has no members - file mentions disabled', …)
zeroMembersWarnedRef.current = true
} |
||
| } | ||
| }, [worktreePath, fileIndex]) | ||
| }, [worktreeId, worktreePath, connectionId, fileIndex, sessionId, activeConnection]) | ||
|
|
||
| // File mentions hook | ||
| const fileMentions = useFileMentions(inputValue, cursorPosition, fileIndex) | ||
|
|
@@ -1903,16 +1947,28 @@ export function SessionView({ sessionId }: SessionViewProps): React.JSX.Element | |
| } else if (session.connection_id) { | ||
| // Connection session: resolve the connection folder path | ||
| setConnectionId(session.connection_id) | ||
| setConnectionPathError(null) // Clear any previous error | ||
| try { | ||
| const connResult = await window.connectionOps.get(session.connection_id) | ||
| if (shouldAbortInit()) return | ||
|
|
||
| if (connResult.success && connResult.connection) { | ||
| wtPath = connResult.connection.path | ||
| setWorktreePath(wtPath) | ||
| transcriptSourceRef.current.worktreePath = wtPath | ||
| setConnectionPathError(null) // Explicitly clear on success | ||
| } else { | ||
| // Connection lookup failed or returned no connection | ||
| const errorMsg = connResult.error || 'Connection not found' | ||
| console.error('Failed to resolve connection path:', errorMsg, 'connectionId:', session.connection_id) | ||
| setConnectionPathError(errorMsg) | ||
| toast.error(`Failed to load connection: ${errorMsg}`) | ||
| } | ||
| } catch { | ||
| console.warn('Failed to resolve connection path for session') | ||
| } catch (error) { | ||
| const errorMsg = error instanceof Error ? error.message : 'Unknown error' | ||
| console.error('Exception resolving connection path:', error, 'connectionId:', session.connection_id) | ||
| setConnectionPathError(errorMsg) | ||
| toast.error(`Failed to load connection: ${errorMsg}`) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -4029,6 +4085,30 @@ export function SessionView({ sessionId }: SessionViewProps): React.JSX.Element | |
| {/* Attachment previews */} | ||
| <AttachmentPreview attachments={attachments} onRemove={handleRemoveAttachment} /> | ||
|
|
||
| {/* Connection error banner */} | ||
| {connectionPathError && ( | ||
| <div className="mx-3 mb-2 p-3 rounded-md bg-destructive/10 border border-destructive/20 flex items-start gap-2"> | ||
| <AlertCircle className="h-4 w-4 text-destructive shrink-0 mt-0.5" /> | ||
| <div className="flex-1 min-w-0"> | ||
| <p className="text-sm text-destructive font-medium">Connection Error</p> | ||
| <p className="text-xs text-muted-foreground mt-1"> | ||
| Failed to load connection path: {connectionPathError}. File mentions (@) will not work. | ||
| </p> | ||
| </div> | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| onClick={() => { | ||
| // Retry connection loading by reinitializing the session | ||
| initializeSession() | ||
| }} | ||
greptile-apps[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| className="shrink-0" | ||
| > | ||
| <RefreshCw className="h-4 w-4" /> | ||
| </Button> | ||
| </div> | ||
| )} | ||
|
|
||
| {/* Middle: textarea */} | ||
| <textarea | ||
| ref={textareaRef} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,10 @@ interface FileTreeState { | |
| // Actions | ||
| loadFileTree: (worktreePath: string) => Promise<void> | ||
| loadFileIndex: (worktreePath: string) => Promise<void> | ||
| loadFileIndexForConnection: ( | ||
| connectionId: string, | ||
| members: Array<{ symlinkName: string; worktreePath: string }> | ||
| ) => Promise<void> | ||
| loadChildren: (worktreePath: string, dirPath: string) => Promise<void> | ||
| setExpanded: (worktreePath: string, paths: Set<string>) => void | ||
| toggleExpanded: (worktreePath: string, path: string) => void | ||
|
|
@@ -127,6 +131,9 @@ export const useFileTreeStore = create<FileTreeState>()( | |
| const loadingMap = new Map(state.fileIndexLoadingByWorktree) | ||
| if (result.success && result.files) { | ||
| indexMap.set(worktreePath, result.files) | ||
| } else { | ||
| // Log failure but don't throw - allows graceful degradation | ||
| console.error('[FileTreeStore] Failed to scan files:', result.error, 'path:', worktreePath) | ||
| } | ||
| loadingMap.set(worktreePath, false) | ||
| return { fileIndexByWorktree: indexMap, fileIndexLoadingByWorktree: loadingMap } | ||
|
|
@@ -136,7 +143,8 @@ export const useFileTreeStore = create<FileTreeState>()( | |
| // updates flow even when the Files sidebar tab is hidden. | ||
| // startWatching guards against duplicate subscriptions internally. | ||
| get().startWatching(worktreePath) | ||
| } catch { | ||
| } catch (error) { | ||
| console.error('[FileTreeStore] Exception scanning files:', error, 'path:', worktreePath) | ||
| set((state) => { | ||
| const loadingMap = new Map(state.fileIndexLoadingByWorktree) | ||
| loadingMap.set(worktreePath, false) | ||
|
|
@@ -145,6 +153,92 @@ export const useFileTreeStore = create<FileTreeState>()( | |
| } | ||
| }, | ||
|
|
||
| // 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 | ||
| } | ||
| }) | ||
yonikliger-velocity marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // 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 | ||
|
Comment on lines
+188
to
+213
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Partial member scan failures stored silently with no user feedback
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 |
||
| } | ||
|
|
||
| // 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 } | ||
| }) | ||
| } | ||
| }, | ||
|
Comment on lines
153
to
+240
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Connection file index never stays fresh — no file watchers set up
Without calling Each member worktree should be watched, and the // After the set() call that stores uniqueFiles…
for (const member of members) {
get().startWatching(member.worktreePath)
}Note that |
||
|
|
||
| // Lazy load children for a directory | ||
| loadChildren: async (worktreePath: string, dirPath: string) => { | ||
| try { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The connection branch only calls
loadFileIndexForConnectionwhilefileIndex === EMPTY_FILE_INDEX, so once a connection index is cached it is never refreshed even ifactiveConnection.memberschanges (for example viaaddMember/removeMember/updateConnectionMembersinuseConnectionStore). 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 👍 / 👎.