From 44d5bed721f764a7cfc5f361d8a5fd7435fa6efb Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 11 Jul 2025 07:52:21 +0200 Subject: [PATCH] Fix bugs in repository discovery feature (#282) --- mac/VibeTunnel/Core/Models/AppConstants.swift | 24 +++-- .../Services/RepositoryDiscoveryService.swift | 61 +++++------- .../Components/NewSessionForm.swift | 24 +++-- .../RepositoryDiscoveryServiceTests.swift | 96 +++++++++++++++++++ 4 files changed, 148 insertions(+), 57 deletions(-) create mode 100644 mac/VibeTunnelTests/RepositoryDiscoveryServiceTests.swift diff --git a/mac/VibeTunnel/Core/Models/AppConstants.swift b/mac/VibeTunnel/Core/Models/AppConstants.swift index ab27ab74..757299e6 100644 --- a/mac/VibeTunnel/Core/Models/AppConstants.swift +++ b/mac/VibeTunnel/Core/Models/AppConstants.swift @@ -16,6 +16,11 @@ enum AppConstants { static let preventSleepWhenRunning = "preventSleepWhenRunning" static let enableScreencapService = "enableScreencapService" static let repositoryBasePath = "repositoryBasePath" + // New Session keys + static let newSessionCommand = "NewSession.command" + static let newSessionWorkingDirectory = "NewSession.workingDirectory" + static let newSessionSpawnWindow = "NewSession.spawnWindow" + static let newSessionTitleMode = "NewSession.titleMode" } /// Default values for UserDefaults @@ -46,21 +51,22 @@ enum AppConstants { /// Helper to get string value with proper default static func stringValue(for key: String) -> String { - // If the key doesn't exist in UserDefaults, return our default + // First check if we have a string value + if let value = UserDefaults.standard.string(forKey: key) { + return value + } + + // If the key doesn't exist at all, return our default if UserDefaults.standard.object(forKey: key) == nil { switch key { case UserDefaultsKeys.repositoryBasePath: - // return last used path if it's exists - if let value = UserDefaults.standard.value(forKey: "NewSession.workingDirectory") as? String { - return value - } else { - return Defaults.repositoryBasePath - } - + return Defaults.repositoryBasePath default: return "" } } - return UserDefaults.standard.string(forKey: key) ?? "" + + // Key exists but contains non-string value, return empty string + return "" } } diff --git a/mac/VibeTunnel/Core/Services/RepositoryDiscoveryService.swift b/mac/VibeTunnel/Core/Services/RepositoryDiscoveryService.swift index bd2fb1f8..cac11ea7 100644 --- a/mac/VibeTunnel/Core/Services/RepositoryDiscoveryService.swift +++ b/mac/VibeTunnel/Core/Services/RepositoryDiscoveryService.swift @@ -65,24 +65,15 @@ public final class RepositoryDiscoveryService { return } - Task.detached { [weak self] in - // Perform discovery in background - let discoveredRepos = await self?.performDiscovery(in: expandedPath) - - guard let discoveredRepos else { - return - } - - await MainActor.run { [weak self] in - // Cache and update results - self?.repositoryCache[expandedPath] = discoveredRepos - self?.repositories = discoveredRepos - - Logger.repositoryDiscovery.info("Discovered \(discoveredRepos.count) repositories in: \(expandedPath)") - - self?.isDiscovering = false - } - } + let discoveredRepos = await self.performDiscovery(in: expandedPath) + + self.isDiscovering = false + + // Cache and update results + self.repositoryCache[expandedPath] = discoveredRepos + self.repositories = discoveredRepos + + Logger.repositoryDiscovery.info("Discovered \(discoveredRepos.count) repositories in: \(expandedPath)") } /// Clear the repository cache @@ -94,32 +85,24 @@ public final class RepositoryDiscoveryService { // MARK: - Private Methods /// Perform the actual discovery work - private nonisolated func performDiscovery(in basePath: String) async -> [DiscoveredRepository] { - return await withTaskGroup(of: [DiscoveredRepository].self) { taskGroup in - var allRepositories: [DiscoveredRepository] = [] - - // Submit discovery task - taskGroup.addTask { [weak self] in - await self?.scanDirectory(basePath, depth: 0) ?? [] - } - - // Collect results - for await repositories in taskGroup { - allRepositories.append(contentsOf: repositories) - } - - // Sort by folder name for consistent display - return allRepositories.sorted { $0.folderName < $1.folderName } - } + private func performDiscovery(in basePath: String) async -> [DiscoveredRepository] { + let allRepositories = await scanDirectory(basePath, depth: 0) + + // Sort by folder name for consistent display + return allRepositories.sorted { $0.folderName < $1.folderName } } /// Recursively scan a directory for Git repositories - private nonisolated func scanDirectory(_ path: String, depth: Int) async -> [DiscoveredRepository] { + private func scanDirectory(_ path: String, depth: Int) async -> [DiscoveredRepository] { guard depth < maxSearchDepth else { Logger.repositoryDiscovery.debug("Max depth reached at: \(path)") return [] } + guard !Task.isCancelled else { + return [] + } + do { let fileManager = FileManager.default let url = URL(fileURLWithPath: path) @@ -170,13 +153,13 @@ public final class RepositoryDiscoveryService { } /// Check if a directory is a Git repository - private nonisolated func isGitRepository(at path: String) -> Bool { + private func isGitRepository(at path: String) -> Bool { let gitPath = URL(fileURLWithPath: path).appendingPathComponent(".git").path return FileManager.default.fileExists(atPath: gitPath) } /// Create a DiscoveredRepository from a path - private nonisolated func createDiscoveredRepository(at path: String) async -> DiscoveredRepository { + private func createDiscoveredRepository(at path: String) async -> DiscoveredRepository { let url = URL(fileURLWithPath: path) let folderName = url.lastPathComponent @@ -195,7 +178,7 @@ public final class RepositoryDiscoveryService { } /// Get the last modified date of a repository - nonisolated private func getLastModifiedDate(at path: String) -> Date { + private func getLastModifiedDate(at path: String) -> Date { do { let attributes = try FileManager.default.attributesOfItem(atPath: path) return attributes[.modificationDate] as? Date ?? Date.distantPast diff --git a/mac/VibeTunnel/Presentation/Components/NewSessionForm.swift b/mac/VibeTunnel/Presentation/Components/NewSessionForm.swift index d913acf7..10d05080 100644 --- a/mac/VibeTunnel/Presentation/Components/NewSessionForm.swift +++ b/mac/VibeTunnel/Presentation/Components/NewSessionForm.swift @@ -474,21 +474,27 @@ struct NewSessionForm: View { // MARK: - Preferences private func loadPreferences() { - if let savedCommand = UserDefaults.standard.string(forKey: "NewSession.command") { + if let savedCommand = UserDefaults.standard.string(forKey: AppConstants.UserDefaultsKeys.newSessionCommand) { command = savedCommand } - workingDirectory = AppConstants.stringValue(for: AppConstants.UserDefaultsKeys.repositoryBasePath) + // Restore last used working directory, not repository base path + if let savedDirectory = UserDefaults.standard.string(forKey: AppConstants.UserDefaultsKeys.newSessionWorkingDirectory) { + workingDirectory = savedDirectory + } else { + // Default to home directory if never set + workingDirectory = "~/" + } // Check if spawn window preference has been explicitly set - if UserDefaults.standard.object(forKey: "NewSession.spawnWindow") != nil { - spawnWindow = UserDefaults.standard.bool(forKey: "NewSession.spawnWindow") + if UserDefaults.standard.object(forKey: AppConstants.UserDefaultsKeys.newSessionSpawnWindow) != nil { + spawnWindow = UserDefaults.standard.bool(forKey: AppConstants.UserDefaultsKeys.newSessionSpawnWindow) } else { // Default to true if never set spawnWindow = true } - if let savedMode = UserDefaults.standard.string(forKey: "NewSession.titleMode"), + if let savedMode = UserDefaults.standard.string(forKey: AppConstants.UserDefaultsKeys.newSessionTitleMode), let mode = TitleMode(rawValue: savedMode) { titleMode = mode @@ -496,10 +502,10 @@ struct NewSessionForm: View { } private func savePreferences() { - UserDefaults.standard.set(command, forKey: "NewSession.command") - UserDefaults.standard.set(workingDirectory, forKey: "NewSession.workingDirectory") - UserDefaults.standard.set(spawnWindow, forKey: "NewSession.spawnWindow") - UserDefaults.standard.set(titleMode.rawValue, forKey: "NewSession.titleMode") + UserDefaults.standard.set(command, forKey: AppConstants.UserDefaultsKeys.newSessionCommand) + UserDefaults.standard.set(workingDirectory, forKey: AppConstants.UserDefaultsKeys.newSessionWorkingDirectory) + UserDefaults.standard.set(spawnWindow, forKey: AppConstants.UserDefaultsKeys.newSessionSpawnWindow) + UserDefaults.standard.set(titleMode.rawValue, forKey: AppConstants.UserDefaultsKeys.newSessionTitleMode) } } diff --git a/mac/VibeTunnelTests/RepositoryDiscoveryServiceTests.swift b/mac/VibeTunnelTests/RepositoryDiscoveryServiceTests.swift new file mode 100644 index 00000000..dbea5308 --- /dev/null +++ b/mac/VibeTunnelTests/RepositoryDiscoveryServiceTests.swift @@ -0,0 +1,96 @@ +import Testing +import Foundation +@testable import VibeTunnel + +@Suite("RepositoryDiscoveryService Tests") +struct RepositoryDiscoveryServiceTests { + + @Test("Test repository discovery initialization") + @MainActor + func testServiceInitialization() async { + let service = RepositoryDiscoveryService() + + #expect(service.repositories.isEmpty) + #expect(!service.isDiscovering) + #expect(service.lastError == nil) + } + + @Test("Test discovery state management") + @MainActor + func testDiscoveryStateManagement() async { + let service = RepositoryDiscoveryService() + + // Start discovery + let task = Task { + await service.discoverRepositories(in: "/nonexistent/path") + } + + // Give it a moment to start + try? await Task.sleep(nanoseconds: 100_000_000) // 0.1 seconds + + // Should not start another discovery while one is in progress + await service.discoverRepositories(in: "/another/path") + + // Wait for completion + await task.value + + // Should eventually reset isDiscovering + #expect(!service.isDiscovering) + } + + @Test("Test cache functionality") + @MainActor + func testCacheFunctionality() async throws { + let service = RepositoryDiscoveryService() + let testPath = NSTemporaryDirectory() + + // First discovery + await service.discoverRepositories(in: testPath) + try await Task.sleep(nanoseconds: 500_000_000) // 0.5 seconds + let firstCount = service.repositories.count + + // Clear cache + service.clearCache() + + // Second discovery should potentially find different results + await service.discoverRepositories(in: testPath) + try await Task.sleep(nanoseconds: 500_000_000) // 0.5 seconds + + // Results should be consistent for the same path + #expect(service.repositories.count == firstCount) + } + + @Test("Test race condition handling") + @MainActor + func testRaceConditionHandling() async throws { + // Create a service that will be deallocated during discovery + var service: RepositoryDiscoveryService? = RepositoryDiscoveryService() + + // Start discovery + Task { + await service?.discoverRepositories(in: NSTemporaryDirectory()) + } + + // Deallocate service while discovery might be in progress + try await Task.sleep(nanoseconds: 50_000_000) // 0.05 seconds + service = nil + + // Wait a bit more to ensure the task completes + try await Task.sleep(nanoseconds: 500_000_000) // 0.5 seconds + + // Test passes if no crash occurs and the flag is properly reset + #expect(true) // If we get here, the race condition was handled + } + + @Test("Test tilde expansion in path") + @MainActor + func testTildeExpansion() async { + let service = RepositoryDiscoveryService() + + // Test with tilde path + await service.discoverRepositories(in: "~/") + + // The service should handle tilde expansion without errors + #expect(service.lastError == nil) + } +} \ No newline at end of file