From 68135e1a83d66d4865a26eaff420fa8064b87981 Mon Sep 17 00:00:00 2001 From: galz10 Date: Tue, 27 Jan 2026 14:56:12 -0800 Subject: [PATCH] fix: pr feedback --- packages/cli/src/config/trustedFolders.ts | 30 ++++-- .../src/config/trustedFoldersCache.test.ts | 91 +++++++++++++++++++ packages/cli/src/ui/AppContainer.tsx | 2 +- 3 files changed, 115 insertions(+), 8 deletions(-) create mode 100644 packages/cli/src/config/trustedFoldersCache.test.ts diff --git a/packages/cli/src/config/trustedFolders.ts b/packages/cli/src/config/trustedFolders.ts index 9badef3271..a546995417 100644 --- a/packages/cli/src/config/trustedFolders.ts +++ b/packages/cli/src/config/trustedFolders.ts @@ -63,15 +63,30 @@ export interface TrustResult { source: 'ide' | 'file' | undefined; } +const realPathCache = new Map(); + +/** + * FOR TESTING PURPOSES ONLY. + * Clears the real path cache. + */ +export function clearRealPathCacheForTesting(): void { + realPathCache.clear(); +} + function getRealPath(location: string): string { - try { - if (fs.existsSync(location)) { - return fs.realpathSync(location); - } - } catch (_e) { - // Fallback to unresolved path + let realPath = realPathCache.get(location); + if (realPath !== undefined) { + return realPath; } - return location; + + try { + realPath = fs.existsSync(location) ? fs.realpathSync(location) : location; + } catch { + realPath = location; + } + + realPathCache.set(location, realPath); + return realPath; } export class LoadedTrustedFolders { @@ -158,6 +173,7 @@ let loadedTrustedFolders: LoadedTrustedFolders | undefined; */ export function resetTrustedFoldersForTesting(): void { loadedTrustedFolders = undefined; + clearRealPathCacheForTesting(); } export function loadTrustedFolders(): LoadedTrustedFolders { diff --git a/packages/cli/src/config/trustedFoldersCache.test.ts b/packages/cli/src/config/trustedFoldersCache.test.ts new file mode 100644 index 0000000000..03f916b990 --- /dev/null +++ b/packages/cli/src/config/trustedFoldersCache.test.ts @@ -0,0 +1,91 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import * as fs from 'node:fs'; +import { + loadTrustedFolders, + TrustLevel, + resetTrustedFoldersForTesting, +} from './trustedFolders.js'; + +vi.mock('fs', async (importOriginal) => { + const actualFs = await importOriginal(); + return { + ...actualFs, + existsSync: vi.fn(), + realpathSync: vi.fn(), + readFileSync: vi.fn(), + }; +}); + +describe('Trusted Folders realpath caching', () => { + beforeEach(() => { + resetTrustedFoldersForTesting(); + vi.resetAllMocks(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('should only call fs.realpathSync once for the same path', () => { + const mockPath = '/some/path'; + const mockRealPath = '/real/path'; + + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.realpathSync).mockReturnValue(mockRealPath); + vi.mocked(fs.readFileSync).mockReturnValue( + JSON.stringify({ + [mockPath]: TrustLevel.TRUST_FOLDER, + '/another/path': TrustLevel.TRUST_FOLDER, + }), + ); + + const folders = loadTrustedFolders(); + + // Call isPathTrusted multiple times with the same path + folders.isPathTrusted(mockPath); + folders.isPathTrusted(mockPath); + folders.isPathTrusted(mockPath); + + // fs.realpathSync should only be called once for mockPath (at the start of isPathTrusted) + // And once for each rule in the config (if they are different) + + // Let's check calls for mockPath + const realpathCalls = vi.mocked(fs.realpathSync).mock.calls; + const mockPathCalls = realpathCalls.filter((call) => call[0] === mockPath); + + expect(mockPathCalls.length).toBe(1); + }); + + it('should cache results for rule paths in the loop', () => { + const rulePath = '/rule/path'; + const locationPath = '/location/path'; + + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.realpathSync).mockImplementation((p) => p as string); // identity for simplicity + vi.mocked(fs.readFileSync).mockReturnValue( + JSON.stringify({ + [rulePath]: TrustLevel.TRUST_FOLDER, + }), + ); + + const folders = loadTrustedFolders(); + + // First call + folders.isPathTrusted(locationPath); + const firstCallCount = vi.mocked(fs.realpathSync).mock.calls.length; + expect(firstCallCount).toBe(2); // locationPath and rulePath + + // Second call with same location and same config + folders.isPathTrusted(locationPath); + const secondCallCount = vi.mocked(fs.realpathSync).mock.calls.length; + + // Should still be 2 because both were cached + expect(secondCallCount).toBe(2); + }); +}); diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index 45ccd33ad0..d94468a1f5 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -213,7 +213,7 @@ export const AppContainer = (props: AppContainerProps) => { const activeHooks = useHookDisplayState(); const [updateInfo, setUpdateInfo] = useState(null); const [isTrustedFolder, setIsTrustedFolder] = useState( - isWorkspaceTrusted(settings.merged).isTrusted, + () => isWorkspaceTrusted(settings.merged).isTrusted, ); const [queueErrorMessage, setQueueErrorMessage] = useState(