From 1c0acb65fe9fdb37b65e5b68a31ac08655bbdabd Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 23 Oct 2026 19:21:48 -0700 Subject: [PATCH] fix(explorer): preserve opened folder links across refreshes (#32587) With automatic symlink following disabled, re-reading a parent lists its links as files and discards the directory flag set when a user opened one. Keep that flag while a successful same-owner child listing is cached, in both direct reads and expanded-directory refreshes. Mark inferred folder rows separately so a failed child read can clear them without changing host-confirmed directories. Preserve normal deleted-link and missing-target recovery behavior. Add parent/full refresh, failed read, ownership and Windows drive-root regression coverage. Refs #29069. Co-authored-by: Arnobrizwan <147672066+Arnobrizwan@users.noreply.github.com> --- .../file-explorer-directory-listing.test.ts | 118 +++++++++++++++++- .../file-explorer-directory-listing.ts | 58 ++++++++- ...ile-explorer-expanded-dirs-refresh.test.ts | 42 +++++++ .../file-explorer-expanded-dirs-refresh.ts | 19 ++- .../file-explorer-linked-recovery.test.tsx | 38 ++++++ .../right-sidebar/file-explorer-types.ts | 2 + .../right-sidebar/useFileExplorerTree.ts | 34 +++-- 7 files changed, 297 insertions(+), 14 deletions(-) diff --git a/src/renderer/src/components/right-sidebar/file-explorer-directory-listing.test.ts b/src/renderer/src/components/right-sidebar/file-explorer-directory-listing.test.ts index 9495453e89a..ec84de33dc0 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-directory-listing.test.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-directory-listing.test.ts @@ -10,7 +10,12 @@ vi.mock('./file-explorer-operation-owner', () => ({ getFileExplorerOwnerUnresolvedMessage: () => 'unresolved' })) -import { readFileExplorerDirectory } from './file-explorer-directory-listing' +import { + keepOpenedDirectoryLinks, + clearFailedOpenedDirectoryLink, + readFileExplorerDirectory +} from './file-explorer-directory-listing' +import type { DirCache, TreeNode } from './file-explorer-types' describe('readFileExplorerDirectory', () => { it('re-sorts backend order — remote-runtime and paired-web routes return the host order verbatim', async () => { @@ -39,3 +44,114 @@ it('preserves the host directory classification with default Quick Open preferen expect(entries).toEqual([{ name: 'linked', isDirectory: true, isSymlink: true }]) expect(readRuntimeDirectory).toHaveBeenLastCalledWith(expect.anything(), '/w', undefined) }) + +describe('keepOpenedDirectoryLinks', () => { + const node = (name: string, overrides: Partial = {}): TreeNode => ({ + name, + path: `/w/${name}`, + relativePath: name, + isDirectory: false, + depth: 0, + operationOwner: { kind: 'local' }, + ...overrides + }) + + it('keeps an opened folder link folder-shaped when the parent re-read lists it as a file', () => { + const previous = [node('linked', { isSymlink: true, isDirectory: true }), node('other')] + const next = [node('linked', { isSymlink: true }), node('other')] + const kept = keepOpenedDirectoryLinks(previous, next, { + '/w/linked': { children: [], operationOwner: { kind: 'local' } } + }) + expect(kept.map((child) => [child.name, child.isDirectory])).toEqual([ + ['linked', true], + ['other', false] + ]) + }) + + it('lets a link revert once its own listing is no longer cached', () => { + const previous = [node('linked', { isSymlink: true, isDirectory: true })] + const next = [node('linked', { isSymlink: true })] + expect(keepOpenedDirectoryLinks(previous, next, {})).toBe(next) + }) + + it('never promotes a link that was not opened, or a plain file with the same path', () => { + const previous = [node('linked', { isSymlink: true }), node('file', { isDirectory: true })] + const next = [node('linked', { isSymlink: true }), node('file')] + const cache = { '/w/linked': { children: [] }, '/w/file': { children: [] } } + expect(keepOpenedDirectoryLinks(previous, next, cache)).toBe(next) + }) + + it('does not restore a deleted link or a link replaced by a regular file', () => { + const previous = [node('linked', { isSymlink: true, isDirectory: true })] + const cache = { '/w/linked': { children: [], operationOwner: { kind: 'local' as const } } } + expect(keepOpenedDirectoryLinks(previous, [], cache)).toEqual([]) + expect(keepOpenedDirectoryLinks(previous, [node('linked')], cache)[0].isDirectory).toBe(false) + }) + + it('does not reuse a failed or still-unproven child listing', () => { + const previous = [node('linked', { isSymlink: true, isDirectory: true })] + const next = [node('linked', { isSymlink: true })] + expect(keepOpenedDirectoryLinks(previous, next, { '/w/linked': { children: [] } })).toBe(next) + expect( + keepOpenedDirectoryLinks(previous, next, { + '/w/linked': { children: [], operationOwner: { kind: 'local' }, error: 'Not a directory' } + }) + ).toBe(next) + }) + + it('does not carry directory classification across operation owners', () => { + const previous = [node('linked', { isSymlink: true, isDirectory: true })] + const localCache = { + '/w/linked': { children: [], operationOwner: { kind: 'local' as const } } + } + const remoteNode = node('linked', { + isSymlink: true, + operationOwner: { kind: 'ssh', connectionId: 'server-2' } + }) + expect(keepOpenedDirectoryLinks(previous, [remoteNode], localCache)[0].isDirectory).toBe(false) + const next = [node('linked', { isSymlink: true })] + expect( + keepOpenedDirectoryLinks(previous, next, { + '/w/linked': { children: [], operationOwner: remoteNode.operationOwner } + }) + ).toBe(next) + }) + + it('leaves unrelated unowned links untouched while preserving a known folder', () => { + const previous = [node('linked', { isSymlink: true, isDirectory: true })] + const next = [ + node('linked', { isSymlink: true }), + node('unowned', { isSymlink: true, operationOwner: undefined }) + ] + const result = keepOpenedDirectoryLinks(previous, next, { + '/w/linked': { children: [], operationOwner: { kind: 'local' } } + }) + expect(result.map((child) => child.isDirectory)).toEqual([true, false]) + }) +}) + +describe('clearFailedOpenedDirectoryLink', () => { + it.each(['/w/link', 'C:\\link'])('drops only inferred folder state for failed %s', (linkPath) => { + const parentPath = linkPath.startsWith('C:') ? 'C:\\' : '/w' + const cache: Record = { + [parentPath]: { + children: [ + { + name: 'link', + path: linkPath, + relativePath: 'link', + isDirectory: true, + isSymlink: true, + isDirectoryFromOpenedLink: true, + depth: 0 + } + ] + } + } + expect( + clearFailedOpenedDirectoryLink(cache, linkPath)[parentPath].children[0].isDirectory + ).toBe(false) + cache[parentPath].children[0].isDirectoryFromOpenedLink = false + expect(clearFailedOpenedDirectoryLink(cache, linkPath)).toBe(cache) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/file-explorer-directory-listing.ts b/src/renderer/src/components/right-sidebar/file-explorer-directory-listing.ts index 553eba06a19..e4a4dbaadba 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-directory-listing.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-directory-listing.ts @@ -3,8 +3,9 @@ import { getRelativePathInsideRoot, joinPath } from '@/lib/path' import type { DirEntry } from '../../../../shared/filesystem-entry-types' import { sortDirEntries } from '../../../../shared/file-name-sort' import { readRuntimeDirectory } from '@/runtime/runtime-file-mutation-client' -import type { FileExplorerOperationOwner, TreeNode } from './file-explorer-types' +import type { DirCache, FileExplorerOperationOwner, TreeNode } from './file-explorer-types' import { shouldIncludeFileExplorerEntry } from './file-explorer-entries' +import { parentDirForWatchPath, resolveCachedDirPath } from './file-explorer-watch-path' import { getFileExplorerOperationOwner, getFileExplorerOwnerUnresolvedMessage, @@ -37,6 +38,61 @@ export function fileExplorerEntriesToTreeNodes( }) } +// Without followSymlinks, re-reading a parent lists opened folder links as files (#29069). +export function keepOpenedDirectoryLinks( + previousChildren: readonly TreeNode[] | undefined, + nextChildren: TreeNode[], + dirCache: Readonly> +): TreeNode[] { + const openedLinks = new Map() + for (const child of previousChildren ?? []) { + const cachedLink = dirCache[child.path] + // Why: pending/failed reads and listings from another host aren't proof of an opened folder. + if ( + child.isSymlink && + child.isDirectory && + child.operationOwner && + child.operationOwner.kind !== 'unresolved' && + cachedLink?.operationOwner && + cachedLink.error === undefined && + JSON.stringify(child.operationOwner) === JSON.stringify(cachedLink.operationOwner) + ) { + openedLinks.set(child.path, JSON.stringify(child.operationOwner)) + } + } + if (openedLinks.size === 0) { + return nextChildren + } + return nextChildren.map((child) => + child.isSymlink && + !child.isDirectory && + child.operationOwner !== undefined && + openedLinks.get(child.path) === JSON.stringify(child.operationOwner) + ? { ...child, isDirectory: true, isDirectoryFromOpenedLink: true } + : child + ) +} + +export function clearFailedOpenedDirectoryLink( + cache: Record, + linkPath: string +): Record { + const parentPath = resolveCachedDirPath(cache, parentDirForWatchPath(linkPath)) + if (!parentPath) { + return cache + } + const parent = cache[parentPath] + let changed = false + const children = parent.children.map((child) => { + if (child.path !== linkPath || !child.isSymlink || !child.isDirectoryFromOpenedLink) { + return child + } + changed = true + return { ...child, isDirectory: false, isDirectoryFromOpenedLink: false } + }) + return changed ? { ...cache, [parentPath]: { ...parent, children } } : cache +} + export async function readFileExplorerDirectory( activeWorktreeId: string | null | undefined, worktreePath: string | null, diff --git a/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts index a46014d23f2..56f6bee2d6b 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts @@ -518,4 +518,46 @@ describe('refreshFileExplorerExpandedDirs', () => { expect(readDirectory).toHaveBeenCalledTimes(1) expect(cache['/repo/b']).toEqual(newerBCache) }) + + it('keeps an opened folder link folder-shaped when a refresh re-reads its parent (#29069)', async () => { + let cache: Record = { + '/repo/src': { + children: [ + { + name: 'linked', + path: '/repo/src/linked', + relativePath: 'src/linked', + isDirectory: true, + isSymlink: true, + depth: 1, + operationOwner: { kind: 'local' } + } + ], + operationOwner: { kind: 'local' } + }, + '/repo/src/linked': { children: [], operationOwner: { kind: 'local' } } + } + const setDirCache = vi.fn((update: CacheUpdate) => { + cache = typeof update === 'function' ? update(cache) : update + }) + const { updateLoadingDirPaths } = createLoadingDirPathsRecorder() + + await refreshFileExplorerExpandedDirs({ + dirs: [{ dirPath: '/repo/src', depth: 0 }], + worktreePath: '/repo', + dirLoadTracker: createFileExplorerDirLoadTracker(), + setDirCache, + updateLoadingDirPaths, + readDirectory: async () => ({ + // Without followSymlinks the host reports the link as a file. + entries: [{ name: 'linked', isDirectory: false, isSymlink: true }], + operationOwner: { kind: 'local' as const } + }), + maxConcurrentReads: 4 + }) + + expect(cache['/repo/src'].children).toMatchObject([ + { name: 'linked', isDirectory: true, isSymlink: true } + ]) + }) }) diff --git a/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.ts b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.ts index fcbe2d02092..5b9811b1461 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.ts @@ -3,6 +3,8 @@ import type { DirCache } from './file-explorer-types' import type { FileExplorerDirLoadTracker } from './file-explorer-dir-load-tracker' import { fileExplorerEntriesToTreeNodes, + keepOpenedDirectoryLinks, + clearFailedOpenedDirectoryLink, type FileExplorerDirectoryListing } from './file-explorer-directory-listing' import { forEachWithConcurrency } from '../../../../shared/map-with-concurrency' @@ -96,7 +98,7 @@ export async function refreshFileExplorerExpandedDirs({ } setDirCache((prev) => { - const next = { ...prev } + let next = { ...prev } for (const result of currentResults) { next[result.dirPath] = result.cache.error !== undefined @@ -105,7 +107,20 @@ export async function refreshFileExplorerExpandedDirs({ children: prev[result.dirPath]?.children ?? [], error: result.cache.error } - : result.cache + : { + ...result.cache, + children: keepOpenedDirectoryLinks( + prev[result.dirPath]?.children, + result.cache.children, + prev + ) + } + } + // Why: a parent may have preserved a link before this wave learned its target is gone. + for (const result of currentResults) { + if (result.cache.error !== undefined) { + next = clearFailedOpenedDirectoryLink(next, result.dirPath) + } } return next }) diff --git a/src/renderer/src/components/right-sidebar/file-explorer-linked-recovery.test.tsx b/src/renderer/src/components/right-sidebar/file-explorer-linked-recovery.test.tsx index 2b2077ff8ac..95c640b28f4 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-linked-recovery.test.tsx +++ b/src/renderer/src/components/right-sidebar/file-explorer-linked-recovery.test.tsx @@ -136,3 +136,41 @@ it('a cached link recovers when its missing target directory returns', async () expect(hook.result.current.dirCache['/repo/link'].children[0].name).toBe('fresh.ts') cleanup() }) + +it('keeps a manually opened folder link after parent and full-tree refreshes (#29069)', async () => { + readDirectoryMock + .mockReset() + .mockImplementation(async (_id, _root, path) => + path === '/repo' + ? listing({ name: 'link', isDirectory: false, isSymlink: true }) + : listing(entry('example.txt')) + ) + const hook = renderHook(() => useFileExplorerTree('/repo', new Set(['/repo/link']), 'wt-1')) + await act(async () => { + await hook.result.current.loadDir('/repo', -1) + await hook.result.current.loadDir('/repo/link', 0, { force: true, failOnError: true }) + hook.result.current.markPathAsDirectory('/repo/link') + }) + expect(hook.result.current.rootCache?.children[0].isDirectory).toBe(true) + + await act(async () => { + await hook.result.current.refreshDir('/repo') + }) + expect(hook.result.current.rootCache?.children[0].isDirectory).toBe(true) + + await act(async () => { + await hook.result.current.refreshTree() + }) + expect(hook.result.current.rootCache?.children[0]).toMatchObject({ + isDirectory: true, + isSymlink: true + }) + expect(hook.result.current.dirCache['/repo/link'].children[0].name).toBe('example.txt') + + readDirectoryMock.mockRejectedValueOnce(new Error('ENOTDIR')) + await act(async () => { + await hook.result.current.refreshDir('/repo/link') + }) + expect(hook.result.current.rootCache?.children[0].isDirectory).toBe(false) + expect(hook.result.current.dirCache['/repo/link'].error).toBe('ENOTDIR') +}) diff --git a/src/renderer/src/components/right-sidebar/file-explorer-types.ts b/src/renderer/src/components/right-sidebar/file-explorer-types.ts index 62525e2d834..6290e745923 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-types.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-types.ts @@ -12,6 +12,8 @@ export type TreeNode = { relativePath: string isDirectory: boolean isSymlink?: boolean + /** Inferred from an opened link, not the parent's latest host listing. */ + isDirectoryFromOpenedLink?: boolean depth: number /** Host snapshot that produced this node. Destructive actions fail closed without it. */ operationOwner?: FileExplorerOperationOwner diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerTree.ts b/src/renderer/src/components/right-sidebar/useFileExplorerTree.ts index 6c3bef180e6..19ad6a0dc0f 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerTree.ts +++ b/src/renderer/src/components/right-sidebar/useFileExplorerTree.ts @@ -10,6 +10,8 @@ import { } from './file-explorer-operation-owner' import { fileExplorerEntriesToTreeNodes, + keepOpenedDirectoryLinks, + clearFailedOpenedDirectoryLink, readFileExplorerDirectory } from './file-explorer-directory-listing' import { refreshFileExplorerExpandedDirs } from './file-explorer-expanded-dirs-refresh' @@ -135,7 +137,10 @@ export function useFileExplorerTree( ) setDirCache((prev) => ({ ...prev, - [dirPath]: { children, operationOwner: listing.operationOwner } + [dirPath]: { + children: keepOpenedDirectoryLinks(prev[dirPath]?.children, children, prev), + operationOwner: listing.operationOwner + } })) updateLoadingDirPaths((prev) => clearFileExplorerDirsLoading(prev, [dirPath])) return true @@ -152,14 +157,19 @@ export function useFileExplorerTree( setSourceWorkspaceId(null) rootReadFailedRef.current = true } - setDirCache((prev) => ({ - ...prev, - [dirPath]: { - ...prev[dirPath], - children: prev[dirPath]?.children ?? [], - error: describeFileExplorerReadError(error) - } - })) + setDirCache((prev) => + clearFailedOpenedDirectoryLink( + { + ...prev, + [dirPath]: { + ...prev[dirPath], + children: prev[dirPath]?.children ?? [], + error: describeFileExplorerReadError(error) + } + }, + dirPath + ) + ) updateLoadingDirPaths((prev) => clearFileExplorerDirsLoading(prev, [dirPath])) return !options?.failOnError } @@ -179,7 +189,11 @@ export function useFileExplorerTree( } changed = true cacheChanged = true - return { ...child, isDirectory: true } + return { + ...child, + isDirectory: true, + isDirectoryFromOpenedLink: child.isSymlink === true + } }) next[dirPath] = cacheChanged ? { ...cache, children } : cache }